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

Exclude DaemonSets (PEMs) from modification by a Vizier's nodeSelector #1887

Conversation

ddelnano
Copy link
Member

@ddelnano ddelnano commented May 3, 2024

Summary: Exclude DaemonSets (PEMs) from modification by a Vizier's nodeSelector

This accomplishes part of #1861. The remaining work is to include new additional node selector configuration values that only apply to PEMs -- similar to how pemMemoryLimit and pemMemoryRequest work. From my investigation so far, it appears that adding the PEM specific selectors will require a different implementation (via the vizier yaml templating on the Pixie cloud side) and so I thought staging this in two changes made sense.

Relevant Issues: #1861

Type of change: /kind bug

Test Plan: Skaffolded a vizier operator and verified that setting a kubernetes.io/hostname node selector no longer prevents PEMs from being scheduled on all nodes

Changelog Message: Fixed an issue that caused the Vizier CRD to apply node selectors to pods that should be scheduled on all nodes (DaemonSets)

@ddelnano ddelnano requested a review from a team as a code owner May 3, 2024 18:43
@ddelnano ddelnano requested a review from aimichelle May 3, 2024 18:44
@ddelnano ddelnano force-pushed the ddelnano/exclude-pems-for-vizier-node-selectors branch from b6fab9b to 3add443 Compare May 6, 2024 04:00
@ddelnano ddelnano requested a review from a team as a code owner May 6, 2024 04:00
.golangci.yaml Outdated
- path: .*slices\/sort.go
linters:
- typecheck
text: "^(undefined: (min|max))"
Copy link
Member Author

@ddelnano ddelnano May 6, 2024

Choose a reason for hiding this comment

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

I considered upgrading golangci-lint in this PR. While upgrading fixed this undefined error from typecheck, there are many other unrelated linting errors introduced by the upgrade. I'll follow up with this upgrade soon.

Here's the original error:

ddelnano@nas-metal-ubuntu22-l1-kvm:~/code/pixie (ddelnano/exclude-pems-for-vizier-node-selectors) $ golangci-lint run '--timeout=5m0s' '--out-format=checkstyle' /home/ddelnano/code/pixie/src/operator/controllers
<?xml version="1.0" encoding="UTF-8"?>

<checkstyle version="5.0">
  <file name="../../../../opt/px_dev/tools/golang/src/slices/sort.go">
    <error column="7" line="67" message="undefined: min" severity="error" source="typecheck"></error>
    <error column="7" line="97" message="undefined: max" severity="error" source="typecheck"></error>
  </file>
</checkstyle>

Signed-off-by: Dom Delnano <ddelnano@gmail.com>
Signed-off-by: Dom Delnano <ddelnano@gmail.com>
@ddelnano ddelnano force-pushed the ddelnano/exclude-pems-for-vizier-node-selectors branch from 3add443 to f581db1 Compare May 6, 2024 04:07
@ddelnano ddelnano merged commit 79886a4 into pixie-io:main May 9, 2024
23 checks passed
@ddelnano ddelnano deleted the ddelnano/exclude-pems-for-vizier-node-selectors branch May 9, 2024 03:12
ddelnano added a commit that referenced this pull request May 24, 2024
…and operator pods (#1913)

Summary: Add node selectors fields in helm chart to control scheduling
of OLM and operator pods

Previously a helm chart user could only specify the node selectors for
the vizier components. This PR allows for applying node selectors to OLM
and the operator. This was feedback from @ashutoshrathore after the
functionality from #1887 was released.

Relevant Issues: #1861

Type of change: /kind feature

Test Plan: Tested the following scenarios
- [x] Used `helm template` to verify that the manifests are the same if
node selectors are not defined
```
$ helm template pixie-operator k8s/operator/helm/ -f k8s/operator/helm/values.yaml --debug  > new.yaml
install.go:200: [debug] Original chart version: ""
install.go:217: [debug] CHART PATH: /home/ddelnano/code/pixie-worktree/k8s/operator/helm

# Switch branches to capture templated output from existing helm chart
$ git checkout main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
$ helm template pixie-operator k8s/operator/helm/ -f k8s/operator/helm/values.yaml --debug  > old.yaml
install.go:200: [debug] Original chart version: ""
install.go:217: [debug] CHART PATH: /home/ddelnano/code/pixie-worktree/k8s/operator/helm

$ diff old.yaml new.yaml
$
```
- [x] Deployed a local version of the helm chart to a cluster and
verified that the node selector values are set properly for pods in the
`olm` and `px-operator` namespace
- [x] Verified that pods failed to schedule if the pinned host was
terminated
```
$ kubectl -n olm get pods
NAME                                READY   STATUS    RESTARTS   AGE
catalog-operator-77554fbc46-zjq8m   0/1     Pending   0          2m13s
olm-operator-76dc499446-d8qvh       0/1     Pending   0          2m11s
$ kubectl -n px-operator get pods
NAME                               READY   STATUS    RESTARTS   AGE
vizier-operator-5ff9749c94-98q87   0/1     Pending   0          2m17s
```

Changelog Message: Add support for specifying node selector in the helm
chart for OLM and operator pods

Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants