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

rollouts: add status conditions to remote root sync #3849

Merged

Conversation

ChristopherFry
Copy link
Contributor

@ChristopherFry ChristopherFry commented Feb 24, 2023

This pull request adds the kstatus Reconciling and Stalled conditions status conditions to the Remote Root Sync.

Copy link
Contributor

@natasha41575 natasha41575 left a comment

Choose a reason for hiding this comment

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

Thanks! Some small nits

rollouts/controllers/remoterootsync_controller.go Outdated Show resolved Hide resolved
rollouts/controllers/remoterootsync_controller.go Outdated Show resolved Hide resolved
rrs.Status.SyncStatus = syncStatus
rrs.Status.SyncCreated = true

meta.RemoveStatusCondition(conditions, conditionReconciling)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a TODO here:
Reconciling or Stalled condition should be updated (or externalSync conditions should be added) by examining syncStatus. SyncStatus could have values Stalled, Reconciling, Error, Pending even when syncError is nil.

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 don't think we want actually do this with these being kstatus conditions. From my understanding, the Reconciling and Stalled conditions (in the case of the remote root sync) should only consider if the root sync is created and up to date with the latest spec as defined by the remote root sync, regardless of the sync status of the root sync. When the Reconciling condition is set to true it implies that the Remote Root Sync still needs to reconcile the root sync with the lates spec from the remote root sync. @mortent can you confirm?

Copy link
Contributor

@droot droot Feb 28, 2023

Choose a reason for hiding this comment

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

if the root sync is created and up to date with the latest spec as defined by the remote root sync

You are right and that's where my comment come from. If you look at checkSyncStatus, it can return Pending status without any error when generation != observedGeneration (meaning rootsync is not up to date with the latest spec of rootsync). I think for other values Stalled, Reconciling, Error, we will have to create an extra condition to track externalSyncStatus or something.

Never mind. I misunderstood what you meant up there :). I will capture out yesterday's discussion in another issue, I think that would be better.

@ChristopherFry ChristopherFry merged commit cff89cf into kptdev:main Feb 28, 2023
@ChristopherFry ChristopherFry deleted the cfry/rollouts-rrs-conditions branch February 28, 2023 22:15
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.

None yet

4 participants