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

Peerpodconfig removal #2027

Merged
merged 4 commits into from
Sep 9, 2024
Merged

Peerpodconfig removal #2027

merged 4 commits into from
Sep 9, 2024

Conversation

bpradipt
Copy link
Member

@bpradipt bpradipt commented Sep 6, 2024 •
edited
Loading

This PR removes peerpodconfig controller and instead moves its functionality of advertising node extended resources for peerpods to CAA daemonset itself.

@bpradipt bpradipt force-pushed the peerpodconfig-removal branch 4 times, most recently from 1133cdc to 2c3a48f Compare September 6, 2024 12:36
@bpradipt bpradipt marked this pull request as ready for review September 6, 2024 12:42
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Sep 6, 2024
stevenhorsman
stevenhorsman reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

This seems like a great change in terms of removing code and simplifying things. Do you see any negatives of have the functionality integrated?
I had question from me about the limit default value and I guess we'll need a follow-up operator PR to reflect the removal as well?
Thanks!

src/cloud-api-adaptor/cmd/cloud-api-adaptor/main.go Outdated Show resolved Hide resolved
@bpradipt bpradipt mentioned this pull request Sep 6, 2024
Open
mkulke
mkulke approved these changes Sep 6, 2024
View reviewed changes
Copy link
Collaborator

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

nit about the number of peerpods per node, other lgtm

@bpradipt
Copy link
Member Author

bpradipt commented Sep 6, 2024

This seems like a great change in terms of removing code and simplifying things. Do you see any negatives of have the functionality integrated?
I had question from me about the limit default value and I guess we'll need a follow-up operator PR to reflect the removal as well?

I strongly feel our default deployments should include webhook deployment along with the limit/node. Further hardening the resource management aspects as we move towards 1.0.
Operator PR will be next to remove peerpodconfig.

@mkulke @stevenhorsman what do you prefer as a good default for the number of peerpods per node?

@bpradipt bpradipt force-pushed the peerpodconfig-removal branch from 2c3a48f to 669b70d Compare September 6, 2024 18:28
stevenhorsman
stevenhorsman approved these changes Sep 7, 2024
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@bpradipt
caa: Advertise node extended resources
04aa51d 
Advertise the node extended resources for peer-pods via
the cloud-api-adaptor daemonset. This removes the dependency from
peerpodconfig-controller.
The number of peer pods per node is provided via the config param
PEERPODS_LIMIT_PER_NODE. Default is 10

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
@bpradipt
install: Add PEERPODS_LIMIT_PER_NODE in kustomization.yaml
c1f7511 
Default is set to 10 per node

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
@bpradipt
rbac: Additional rbac to allow patching of node status
6c159d5 
This is needed for CAA to advertise the per node peer-pods limit

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
@bpradipt
peerpodconfig-ctrl: Remove
7d01a66 
Remove peerpodconfig controller as its functionality is now
included in CAA itself.

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
@bpradipt bpradipt force-pushed the peerpodconfig-removal branch from 669b70d to 7d01a66 Compare September 7, 2024 09:33
@bpradipt
Copy link
Member Author

bpradipt commented Sep 7, 2024

Rebased and pushed, to trigger the e2e CI again.

@bpradipt
Copy link
Member Author

bpradipt commented Sep 7, 2024

The libvirt e2e test got killed due to the time it took.
As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.

@stevenhorsman @mkulke any thoughts?

@stevenhorsman
Copy link
Member

The libvirt e2e test got killed due to the time it took. As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.

@stevenhorsman @mkulke any thoughts?

If we need to bump to 1.22.7 for this fix:

Vulnerability #1: GO-2024-3106
    Stack exhaustion in Decoder.Decode in encoding/gob
  More info: https://pkg.go.dev/vuln/GO-2024-3106
  Standard library
    Found in: encoding/gob@go1.21.12
    Fixed in: encoding/gob@go1.22.7

I'm not sure if dependabot will do this as it bumps packages rather than the go version? I'm happy for this to merge without that fix as then if dependabot hasn't created anything by Monday I can manually bump and try things out?

@bpradipt
Copy link
Member Author

bpradipt commented Sep 9, 2024

The libvirt e2e test got killed due to the time it took. As for the govulncheck, there is a vulnerability report for gob.Decoder.Decode. Wonder if we should wait for dependabot alerts and handle it accordingly.
@stevenhorsman @mkulke any thoughts?

If we need to bump to 1.22.7 for this fix:

Vulnerability #1: GO-2024-3106
    Stack exhaustion in Decoder.Decode in encoding/gob
  More info: https://pkg.go.dev/vuln/GO-2024-3106
  Standard library
    Found in: encoding/gob@go1.21.12
    Fixed in: encoding/gob@go1.22.7

I'm not sure if dependabot will do this as it bumps packages rather than the go version? I'm happy for this to merge without that fix as then if dependabot hasn't created anything by Monday I can manually bump and try things out?

I didn't read that the vulnerability is in go standard library. So we will need to bump golang.

I will merge this and open a new PR to bump golang version across the project

@bpradipt bpradipt merged commit 059f642 into confidential-containers:main Sep 9, 2024
26 of 28 checks passed
@bpradipt bpradipt deleted the peerpodconfig-removal branch September 9, 2024 03:44
@bpradipt
Copy link
Member Author

bpradipt commented Sep 9, 2024

Working on the PR to update golang - #2030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevenhorsman stevenhorsman stevenhorsman approved these changes

@mkulke mkulke mkulke approved these changes

@snir911 snir911 Awaiting requested review from snir911

Assignees
No one assigned
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
CoCo Releases
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bpradipt @stevenhorsman @mkulke