Upgrade controller-runtime to v0.15.0 #2481
Conversation
jdn5126
force-pushed
the
raw_client_upgrade
branch
from
a836831
to
40f2575
Compare
jdn5126
force-pushed
the
raw_client_upgrade
branch
2 times, most recently
from
a125938
to
a6d970f
Compare
jdn5126
force-pushed
the
raw_client_upgrade
branch
from
be6cf36
to
00ad2c6
Compare
jdn5126
force-pushed
the
raw_client_upgrade
branch
from
00ad2c6
to
1ccb21e
Compare
There was a problem hiding this comment.
We should probably check all the replace <packages>, maybe some of them might not be relevant..
or add a comment what dependency it is blocked on so that it can be removed when that gets updated?
There was a problem hiding this comment.
Yeah, that's a good call. I tried to remove the non-containernetworking ones early into this PR, but ran into issues, so I need to mark why the replaces are still needed.
| if err = c.tryAllocateENI(ctx); err == nil { | ||
| c.updateLastNodeIPPoolAction() | ||
| } else { | ||
| // Note that no error is returned if ENI allocation fails. This is because ENI allocation failure should not cause node to be "NotReady". | ||
| log.Debugf("Error trying to allocate ENI: %v", err) | ||
| } | ||
| } else { | ||
| log.Debugf("Skipping ENI allocation as the max ENI limit of %d is already reached (accounting for %d unmanaged ENIs and %d trunk ENIs)", | ||
| c.maxENI, c.unmanagedENI, reserveSlotForTrunkENI) | ||
| log.Debugf("Skipping ENI allocation as the max ENI limit is already reached") |
There was a problem hiding this comment.
Should we keep the detailed log giving the number of enis attached ? I think it will be helpful instead of looking at the instance logs
There was a problem hiding this comment.
I considered that, and maybe it would be good to print in hasRoomForEni, but in general it should be easy enough to find that I figured the overhead was not worth it
What type of PR is this?
enhancement
Which issue does this PR fix:
#2160
What does this PR do / Why do we need it:
The main goal for this PR is to upgrade the
controller-runtimemodule to v0.15.0 and handle all of the breaking API changes. The second goal is to use only a single Kubernetes client, i.e. remove the raw client. This, in combination with improvements in the latestcontroller-runtime, helps us address #2160This PR also fixes and re-enables the IPAMD events test. The issue with the test was a mismatch between
eventsv1.Eventsandcorev1.Events.I also took this opportunity to upgrade some other Go modules that had dependabot alerts.
When reviewing, please focus on the changes in
pkg/k8sapi/k8sutils.go. The rest of the changes are either:controller-runtimeupgradeIf an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
N/A
Testing done on this change:
I manually ran all VPC CNI integration tests and verified that they passed. I also ran the performance integration test to verify that there was no increase in memory consumption by the
aws-nodepod and to see the decrease in API server calls.An integration test job will be scheduled against this PR.
Automation added to e2e:
N/A
Will this PR introduce any new dependencies?:
No
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No, Yes
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
Yes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.