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

Only show relevant conditions for the AzureCluster / AzureMachine #1868

Closed
CecileRobertMichon opened this issue Nov 16, 2021 · 5 comments · Fixed by #2093
Closed

Only show relevant conditions for the AzureCluster / AzureMachine #1868

CecileRobertMichon opened this issue Nov 16, 2021 · 5 comments · Fixed by #2093
Assignees

Comments

@CecileRobertMichon
Copy link
Contributor

When building a cluster, the user should only see conditions that apply to that specific cluster in the Status and as part of summary. For example, if the cluster has no vnet peerings, it should not show a "vnet peerings ready" condition.

This is related to kubernetes-sigs/cluster-api#5624.

Two ways we can go about achieving this:

  1. Remove the WithConditions() option in SetSummary https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/scope/cluster.go#L599. This will ensure the controller only uses conditions that are set to set Ready status.
    Pros: minimal code changes required, no changes required if we add or remove services later
    Cons: external controllers setting conditions on the object may interfere, some glitches are possible where the status temporarily becomes Ready even though not all resources are ready if all the set conditions are true and some conditions haven't been set yet.

  2. Add conditional logic to determine the set of Conditions needed based on the length of the different resource specs (ie. if len(spec) != 0 { // append condition to list of conditions}, sort of similar to what CAPA is doing here: https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/461ef72c3b647e729298ec5507c3387dabf7b015/pkg/cloud/scope/cluster.go#L209-L233
    Pros: well defined set of conditions, Ready status will always be correct
    Cons: more logic required, logic will need to evolve whenever we add/remove services

In both cases, we will also need to change services to only set update the Status if the spec has > 0 resources https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/services/vnetpeerings/vnetpeerings.go#L75

@CecileRobertMichon
Copy link
Contributor Author

/assign @Jont828

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon: GitHub didn't allow me to assign the following users: Jont828.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Jont828

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Jont828
Copy link
Contributor

Jont828 commented Nov 17, 2021

Sure, I'd be happy to take a look!

@Jont828
Copy link
Contributor

Jont828 commented Nov 17, 2021

/assign Jont828

@CecileRobertMichon
Copy link
Contributor Author

@Jont828 could we prioritize this one so we can merge it before releasing v1.2 which includes a lot of new async services? Let me know if you need any help.

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