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.

Sign up for GitHub

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

Jump to bottom

[controller-manager] Add documentation and integration test for CloudProfile controller #6360

Merged
merged 5 commits into from
Jul 21, 2022
Merged

[controller-manager] Add documentation and integration test for CloudProfile controller #6360

merged 5 commits into from
Jul 21, 2022

Conversation

rfranzke
Copy link
Member

@rfranzke rfranzke commented Jul 19, 2022 •
edited by timebertt
Loading

How to categorize this PR?

/area dev-productivity scalability
/kind enhancement

What this PR does / why we need it:

Which issue(s) this PR fixes:
Part of #4251

Special notes for your reviewer:
Generally, we want to follow this cookbook while refactoring existing controllers, however we only decided about this after #6333 got merged:

  1. Add documentation
  2. Add integration test based on envtest
  3. Switch controller to controller-runtime

Hence, this PR is now adding the left-overs.

Release note:

NONE

@gardener-prow gardener-prow bot added area/dev-productivity Developer productivity related (how to improve development) area/scalability Scalability related kind/enhancement Enhancement, improvement, extension cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Jul 19, 2022
@gardener-prow gardener-prow bot requested review from ary1992 and krgostev July 19, 2022 15:30
@rfranzke rfranzke mentioned this pull request Jul 19, 2022
Closed
76 tasks
@gardener-prow gardener-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 19, 2022
krgostev
krgostev reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@krgostev krgostev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2022
@timebertt
Copy link
Member

/assign
I will review this PR and also address the open suggestions, while @rfranzke is absent.

rfranzke and others added 5 commits July 21, 2022 11:24
@rfranzke @timebertt
Add documentation
7cf3c59
@rfranzke @timebertt
Move AddToManager to dedicated add.go file
2957f57
@rfranzke @timebertt
Add integration test
ceec51b
@timebertt
Remove pointer for config
a173f8a 
Specifying a nil config is not a valid thing to do.
Otherwise, `AddToManager` would panic.
Hence, rather take a zero value config and take care of correctly handling unset fields.
This is semantically better, as we can't be sure that the given config struct is a real
(i.e. defaulted) object, e.g. in integration tests we don't do defaulting for the config.
@timebertt
Improve integration test
6d9c32a
timebertt
timebertt reviewed Jul 21, 2022
View reviewed changes
pkg/controllermanager/controller/cloudprofile/reconciler.go Outdated
Comment on lines 94 to 122
// TODO voelzmo - this migration step ensures that all MachineImageVersions in the Cloud Profile contain `docker` in their list of supported Container Runtimes
// This can be removed in a couple of versions. Note that while this is still in here, it is impossible to add an image without `docker` support!
migrationHappened := migrateMachineImageVersionCRISupport(cloudProfile)
Copy link
Member

Choose a reason for hiding this comment

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

Now that we are removing this defaulting in the controller, we would need to add some validation that the CRI list is not empty (like proposed in #4501), as other controllers like the shoot maintenance controller as expecting the CRI list not to be empty:

gardener/pkg/controllermanager/controller/shoot/shoot_maintenance_control.go

Lines 445 to 450 in a18f881

func findCRIByName(wanted gardencorev1beta1.CRIName, cris []gardencorev1beta1.CRI) (gardencorev1beta1.CRI, bool) {
for _, cri := range cris {
if cri.Name == wanted {
return cri, true
}
}

I'm wondering, why this defaulting was not done in API defaulting. I would argue that this was conceptually wrong in the first place.
I think the conceptually correct implementation would be defaulting an empty CRI list to a list that only contains docker. This was the implicit default before.

However, not specifying docker in the CRI list is actually not a valid configuration, as gardener still requires docker to be present on the node, even if containerd is used, ref #4673.
Hence we should have validation for the CRI list, that docker is still in there.
This validation can be removed once #4673 is resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Long story short, I will drop the cleanup commit from this PR and open an issue with my findings so that we can clean it up in a dedicated PR without mixing up too many things in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ref #6375

@timebertt timebertt mentioned this pull request Jul 21, 2022
Closed
@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2022
@timebertt
Copy link
Member

/lgtm
/approve

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Jul 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timebertt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 21, 2022
@gardener-prow gardener-prow bot merged commit d9b00b5 into gardener:master Jul 21, 2022
@rfranzke rfranzke deleted the enh/cpfl-crl branch July 26, 2022 06:55
@rfranzke rfranzke changed the title Add documentation and integration test for CloudProfile controller [controller-manager] Add documentation and integration test for CloudProfile controller Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acumino acumino acumino left review comments

@timebertt timebertt timebertt left review comments

@krgostev krgostev krgostev left review comments

@ary1992 ary1992 Awaiting requested review from ary1992

Assignees

@timebertt timebertt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dev-productivity Developer productivity related (how to improve development) area/scalability Scalability related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rfranzke @timebertt @acumino @krgostev