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

Shoot conditions lastUpdateTime is no longer updated continuously #2888

Merged

Conversation

rfranzke
Copy link
Member

How to categorize this PR?

/area scalability robustness
/kind enhancement
/priority normal

What this PR does / why we need it:
The shoot's .status.conditions[].lastUpdateTime is no longer continuously updated to prevent spamming etcd with too many unnecessary updates.

Special notes for your reviewer:
If the gardenlet does not renew its lease anymore then the GCM will, after a certain threshold, set the shoot conditions to Unknown to indicate that the displayed information is most likely outdated.

Release note:

The shoot's `.status.conditions[].lastUpdateTime` is no longer continuously updated. Note that if the gardenlet does not renew its lease anymore then the gardener-controller-manager will, after a certain threshold, set the `Shoot`'s conditions to `Unknown` to indicate that the displayed information is most likely outdated.

@rfranzke
Copy link
Member Author

/assign @amshuman-kr @shreyas-s-rao @vlerenc

@vlerenc
Copy link
Member

vlerenc commented Sep 24, 2020

Thanks @rfranzke. When we merge it/deploy it to dev, we should check whether it indeed behaves as expected and the no longer updated .status.conditions[].lastUpdateTime will not cause trouble in the components.

WDYT if we no longer update .status.conditions[].lastUpdateTime for shoots:

@rfranzke
Copy link
Member Author

/invite @grolu @petersutter @wyb1 @mliepold

@petersutter
Copy link
Contributor

petersutter commented Sep 24, 2020

we have no logic that depends on .status.conditions[].lastUpdateTime.

This would be the only effect in case it is not set (Last Updated will show nothing), however we have a PR ready to remove this field gardener/dashboard#810

Screenshot 2020-09-24 at 15 55 52

@mvladev
Copy link

mvladev commented Sep 24, 2020

Btw there is already a condition builder which checks if condition really needs to be updated e.g.

bldr, err := helper.NewConditionBuilder(gardencorev1beta1.SeedGardenletReady)
if err != nil {
return err
}
condition := helper.GetCondition(seedCopy.Status.Conditions, gardencorev1beta1.SeedGardenletReady)
if condition != nil {
bldr.WithOldCondition(*condition)
}
bldr.WithStatus(gardencorev1beta1.ConditionTrue)
bldr.WithReason("GardenletReady")
bldr.WithMessage("Gardenlet is posting ready status.")
if newCondition, update := bldr.WithNowFunc(metav1.Now).Build(); update {
seed.Status.Conditions = helper.MergeConditions(seedCopy.Status.Conditions, newCondition)
if err := gardenClient.Client().Status().Update(context.TODO(), seed); err != nil {
return err
}
}

pkg/apis/core/v1beta1/helper/helper.go Outdated Show resolved Hide resolved
amshuman-kr
amshuman-kr previously approved these changes Sep 24, 2020
Copy link

@amshuman-kr amshuman-kr left a comment

Choose a reason for hiding this comment

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

I am not aware of all the details of how these conditions and constraints are used. But looks good in principle :-)

petersutter
petersutter previously approved these changes Sep 24, 2020
@rfranzke
Copy link
Member Author

Btw there is already a condition builder which checks if condition really needs to be updated e.g.

Yes, I know, actually I started using it, but then I faced that the botanist/health_check.go would be adapted as well, resulting in a larger refactoring. I have a WIP branch for it and will continue separately.

vpnachev
vpnachev previously approved these changes Sep 25, 2020
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

pkg/utils/kubernetes/health/health.go Outdated Show resolved Hide resolved
Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@mvladev mvladev left a comment

Choose a reason for hiding this comment

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

/lgtm

@rfranzke rfranzke merged commit 79ed94b into gardener:master Sep 25, 2020
@rfranzke rfranzke deleted the feature/condition-last-update-time branch September 25, 2020 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related area/scalability Scalability related kind/enhancement Enhancement, improvement, extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet