Skip to content

VPA - Injected sidecar e2e test #2795

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
Merged
merged 3 commits into from
Feb 6, 2020
Merged

VPA - Injected sidecar e2e test #2795

merged 3 commits into from
Feb 6, 2020

Conversation

krzysied
Copy link
Contributor

@krzysied krzysied commented Feb 4, 2020

The test creates additional webhook that injects sidecar to hamster pods.
Injected sidecars should be in pod container spec, but should not be listed in VpaObservedContainers label. Injected sidecars should not cause eviction.

ref #2726

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2020
@krzysied krzysied mentioned this pull request Feb 4, 2020
Closed
@krzysied
Copy link
Contributor Author

krzysied commented Feb 4, 2020

/assign @bskiba

bskiba
bskiba suggested changes Feb 5, 2020
View reviewed changes
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

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

I've added some comments to the copied code that you can ignore since they are mostly copied.
Does this actually work with the run-e2e-tests.sh script? I remeber there were issues when I tried extracting the common libs from the v1 and v1beta2 dirs.

vertical-pod-autoscaler/e2e/v1/actuation.go Outdated
)

const (
// TODO(krzysied): Update the image url when agnhost:2.10
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not parse in my head

Copy link
Member

Choose a reason for hiding this comment

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

Also it is still relevant? I see 2.10 below.

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 guess something must have interrupted me and I finished the comment with different thought in my head 🤣
I updated the comment and moved it to the test case.

vertical-pod-autoscaler/e2e/v1/actuation.go

utils.DeployWebhookAndService(f, agnhostImage, context, servicePort, containerPort, sidecarParam)

// Webhook must be placed after vpa webhook. Webhooks are registered alphabetically.
Copy link
Member

Choose a reason for hiding this comment

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

Is this really true that Webhooks are registered alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/kubernetes/kubernetes/blob/d90dd93855ab6c3ccda73dfbd3d19c81f56abca5/staging/src/k8s.io/apiserver/pkg/admission/configuration/mutating_webhook_manager.go#L86

Copy link
Member

Choose a reason for hiding this comment

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

That is horrible, thanks for pointing this out to me.

vertical-pod-autoscaler/e2e/utils/certs.go Outdated

// SetupServerCert setups up the server cert. For example, user apiservers and admission webhooks
// can use the cert to prove their identify to the kube-apiserver
func SetupServerCert(namespaceName, serviceName string) *certContext {
Copy link
Member

Choose a reason for hiding this comment

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

Since we only use it for Webhook, maybe name is SetupWebhookCert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I removed serviceName parameter if we only use it for the specific webhook.

bskiba
bskiba reviewed Feb 5, 2020
View reviewed changes
vertical-pod-autoscaler/e2e/v1/actuation.go Outdated

ginkgo.By("Adding recommendation for " + sidecarName)

vpa, err := getVpaClientSet(f).AutoscalingV1().VerticalPodAutoscalers(f.Namespace.Name).
Copy link
Member

Choose a reason for hiding this comment

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

Why not set recommendation up in the first place?

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 though it will be less code with creat + patch. It turned out to be not the greatest idea...
I moved the sidecar recommendation to the hamster vpa setup.

@bskiba
Copy link
Member

bskiba commented Feb 6, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 6, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1d2f183 into kubernetes:master Feb 6, 2020
@krzysied krzysied deleted the vpa_webhook_test branch February 6, 2020 12:32
@jbartosik jbartosik mentioned this pull request May 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbartosik jbartosik Awaiting requested review from jbartosik

1 more reviewer

@bskiba bskiba bskiba approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bskiba bskiba

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@krzysied @bskiba @k8s-ci-robot