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

VPA: Missing Flag Validation in VPA e2e Tests #7723

Open
2 tasks
omerap12 opened this issue Jan 19, 2025 · 5 comments
Open
2 tasks

VPA: Missing Flag Validation in VPA e2e Tests #7723

omerap12 opened this issue Jan 19, 2025 · 5 comments
Labels
area/vertical-pod-autoscaler help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@omerap12
Copy link
Member

omerap12 commented Jan 19, 2025

/area vertical-pod-autoscaler

The VPA components (e.g., recommender, updater, and admission controller) have various configurable flags. For instance, some of the recommender's flags can be found here.

However, the current VPA end-to-end (e2e) tests do not validate the behavior of these flags. Instead, they rely solely on the default flag values without testing any custom configurations. Specifically, the e2e tests deploy VPA components using a predefined deployment script (deploy-for-e2e.sh), which, for example, references the following deployment configuration for the recommender:

---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: vpa-recommender
  namespace: kube-system
spec:
  replicas: 1
  selector:
    matchLabels:
      app: vpa-recommender
  template:
    metadata:
      labels:
        app: vpa-recommender
    spec:
      serviceAccountName: vpa-recommender
      securityContext:
        runAsNonRoot: true
        runAsUser: 65534 # nobody
      containers:
      - name: recommender
        image: registry.k8s.io/autoscaling/vpa-recommender:1.2.2
        imagePullPolicy: IfNotPresent
        resources:
          limits:
            cpu: 200m
            memory: 1000Mi
          requests:
            cpu: 50m
            memory: 500Mi
        ports:
        - name: prometheus
          containerPort: 8942

The above configuration, as seen in the recommender deployment template, does not pass any flags to the recommender component.

To ensure comprehensive testing and avoid potential issues, the e2e tests should:

  1. Incorporate additional deployments of VPA components with various flag configurations.
  2. Validate the behavior of these components under different flag settings.

This enhancement would help identify and prevent regressions, such as this pr, where the namespace filtering behavior was unintentionally changed from a whitelist (vpaObjectNamespace) to a blacklist (ignoredVPANamespaces).

@phuhung273
Copy link

While trying to add the test, can find that even the run-e2e-locally.sh take too much time to run, eg: recommender took 26mins on my machine.

For testing all the flags, we would need lots of deployment config, repeating the ginkgo test will be too expensive in that case. While some of the flags dont even need ginkgo, just simply deploy and some kubectl.

Im thinking of adding another kind of test called quick-e2e-test for some simple flag. Want to know your thought on this @omerap12.

@omerap12
Copy link
Member Author

@phuhung273, I agree. Testing some flags this way sounds better and more efficient, like integration tests. Can you create a PR for one flag so we can take a look? (Though for some flags, we will still need to extend our e2e testing.)

@omerap12
Copy link
Member Author

/triage accepted
/help

@k8s-ci-robot
Copy link
Contributor

@omerap12:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 23, 2025
@phuhung273
Copy link

phuhung273 commented Jan 25, 2025

PR created for vpa-object-namespace and ignored-vpa-object-namespaces. May I have your review @omerap12, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

3 participants