Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce ConsumedCapacity into the SpaceProvisionerConfig status #450

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Nov 14, 2024

Description

Introduce ConsumedCapacity into the SpaceProvisionerConfig status that is meant to mirror the relevant information from the ToolchainStatus for easier inspection of the cluster conditions.

Checks

  1. Did you run make generate target? yes

  2. Did make generate change anything in other projects (host-operator, member-operator)? yes

  3. In case of new CRD, did you the following? NA

  4. In case other projects are changed, please provides PR links.

…t is

meant to mirror the relevant information from the ToolchainStatus for
easier inspection of the cluster conditions.
api/v1alpha1/spaceprovisionerconfig_types.go Show resolved Hide resolved
Comment on lines 59 to 60
// MemoryUsagePercentPerNodeRole is the percent of the memory used per node role (eg. worker, master)
MemoryUsagePercentPerNodeRole map[string]int `json:"memoryUsagePercentPerNode"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong but it should require also these markers

Suggested change
// MemoryUsagePercentPerNodeRole is the percent of the memory used per node role (eg. worker, master)
MemoryUsagePercentPerNodeRole map[string]int `json:"memoryUsagePercentPerNode"`
// MemoryUsagePercentPerNodeRole is the percent of the memory used per node role (eg. worker, master)
// +optional
// +mapType=atomic
MemoryUsagePercentPerNodeRole map[string]int `json:"memoryUsagePercentPerNode,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add the +maptype=atomic, thanks for pointing it out. As for optional and omitempty, I don't these should be added in this case, see my reply to your comment on the SpaceCount. That said, I also noticed the discrepancy between the field name in go and json, so I'll fix that, too :)

Comment on lines +62 to +63
// SpaceCount is the number of spaces currently deployed to the cluster
SpaceCount int `json:"spaceCount"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - I usually saw that if the parent struct is optional also the fields inside are optional

Suggested change
// SpaceCount is the number of spaces currently deployed to the cluster
SpaceCount int `json:"spaceCount"`
// SpaceCount is the number of spaces currently deployed to the cluster
// +optional
SpaceCount int `json:"spaceCount,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this is that we either have a complete capacity report or none at all. Optional and omitempty only influence the JSON that is marshalled or presented to the user and since this is a status and users are not supposed to edit it, I wouldn't find it useful to provide partial capacity reports or as a user to not see "spaceCount: 0" in the status.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so we will always have both fields SpaceCount and MemoryUsagePercentPerNodeRole populated? There cannot be a situation or timeframe when only one is present ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, we will assign the consumed capacity as a whole, not the individual fields.

Copy link

sonarcloud bot commented Nov 15, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants