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

fix(selector): apply component labels/selectors #197

Open
wants to merge 4 commits into
base: separate-db-storage
Choose a base branch
from
Open

fix(selector): apply component labels/selectors #197

wants to merge 4 commits into from

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Sep 17, 2024 •
edited
Loading

Based on #191
Depends on #192
Fixes #194

@andrewazores andrewazores requested a review from a team September 17, 2024 18:18
@andrewazores andrewazores changed the base branch from main to separate-db-storage September 17, 2024 21:12
tthvo
tthvo reviewed Sep 18, 2024
View reviewed changes
Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

We also need to update the NOTES.txt for the port-forward case to include the component label.

cryostat-helm/charts/cryostat/templates/NOTES.txt

Line 41 in fdbc6b9

export POD_NAME=$(kubectl get pods -n {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "cryostat.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" --sort-by=.metadata.creationTimestamp -o jsonpath="{.items[-1:].metadata.name}")

Otherwise, either the database or storage pod might also be selected.

@andrewazores andrewazores force-pushed the selector-labels branch 2 times, most recently from 6270a38 to a7a598e Compare September 18, 2024 14:31
@andrewazores
Copy link
Member Author

I think this change also fixes the helm test flakiness seen in #192.

@tthvo tthvo mentioned this pull request Sep 19, 2024
Merged
@andrewazores andrewazores marked this pull request as ready for review September 19, 2024 21:18
tthvo
tthvo reviewed Sep 20, 2024
View reviewed changes
charts/cryostat/templates/NOTES.txt
@@ -38,7 +38,7 @@
```
kubectl -n {{ .Release.Namespace }} wait --for=condition=available --timeout=60s deploy/{{ include "cryostat.fullname" . }}

export POD_NAME=$(kubectl get pods -n {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "cryostat.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" --sort-by=.metadata.creationTimestamp -o jsonpath="{.items[-1:].metadata.name}")
export POD_NAME=$(kubectl get pods -n {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "cryostat.name" . }},app.kubernetes.io/instance={{ .Release.Name }}",app.kubernetes.io/component=cryostat --sort-by=.metadata.creationTimestamp -o jsonpath="{.items[-1:].metadata.name}")
Copy link
Member

Choose a reason for hiding this comment

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

I think the unit tests need updating after this change :D

charts/cryostat/templates/cryostat_deployment.yaml
spec:
replicas: 1
strategy:
type: Recreate
selector:
matchLabels:
{{- include "cryostat.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: cryostat
Copy link
Member

Choose a reason for hiding this comment

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

Since the selector is an immutable field, changes to this field do cause the upgrades (i.e. helm upgrade) to fail. There is an option for helm upgrade --force to force upgrade with replacement strategy with new objects. This seems pretty destructive though.

Any thoughts on this? It seems we can no longer test the upgrade path without --force.

Copy link
Member

Choose a reason for hiding this comment

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

Related discussion: helm/helm#7082

charts/cryostat/templates/reports_service.yaml
@@ -13,4 +13,5 @@ spec:
- port: {{ .Values.reports.service.httpPort }}
Copy link
Member

Choose a reason for hiding this comment

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

The report's Service should also have label app.kubernetes.io/component: reports for consistency?

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

@tthvo tthvo tthvo left review comments

Assignees
No one assigned
Labels
fix safe-to-test
Projects
4.0.0 release
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] HTTP connection flakiness between components
2 participants
@andrewazores @tthvo