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

Observed generation #637

Merged
merged 4 commits into from
Apr 9, 2021
Merged

Observed generation #637

merged 4 commits into from
Apr 9, 2021

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Mar 15, 2021

This closes #311

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

  • Add status.observedGeneration to rabbitmqcluster
  • Only set status.observedGeneration to the generation of the resource when the controller has successfully reconciled the version
  • Will squash when merging

Additional Context

Local Testing

@ChunyiLyu ChunyiLyu force-pushed the observedGeneration branch from ccef4e4 to a67fe5a Compare March 15, 2021 11:56
@ChunyiLyu ChunyiLyu marked this pull request as draft March 17, 2021 09:46
@ChunyiLyu
Copy link
Contributor Author

Needs to make the test less flaky. Putting is down in favor of messaging topology operator, will pick up in couple days.

@ChunyiLyu ChunyiLyu marked this pull request as ready for review April 7, 2021 16:03
Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Two things that come to mind:

  1. Do we really need to test this at system level? I'll be much more in favour of testing this at integration level with testEnv if it's possible.
  2. Does it make sense to add a test for observed generation transitions/increments?
    It might be a bit in the edge between testing Kubernetes and testing our code, but I'd make an argument to say that we want to test that any update to the generation in metadata is reflected in the status of RabbitMQ.

@ChunyiLyu
Copy link
Contributor Author

ChunyiLyu commented Apr 9, 2021

  1. Do we really need to test this at system level? I'll be much more in favour of testing this at integration level with testEnv if it's possible.

Sadly you cannot... Observed generation is set only after a successful reconcile. Our integration tests controller never successfully exist because no pods ever becomes ready. It's the same as the status ReconcileSuccess where it can only be tested in an actual k8s cluster :(((((

  1. Does it make sense to add a test for observed generation transitions/increments?

Assuming that k8s api is updating the metadata.generation successfully (which is not in scope for our operator), the system test is testing that observedGeneration is set to a correct value: https://github.com/rabbitmq/cluster-operator/pull/637/files#diff-b18e65a50201ede1c97f44776a33955bd5c2dad93e055102833ddebe07fa647bR114. It's enough imo because we are not incrementing the counter ourselves, but rather setting it to the value of the object generation.

@ChunyiLyu ChunyiLyu merged commit 6dbc83d into main Apr 9, 2021
@ChunyiLyu ChunyiLyu deleted the observedGeneration branch April 9, 2021 11:19
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.

RabbitmqCluster lacks status.observedGeneration
3 participants