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

Remove dynakube CR generation from helm chart #745

Merged
merged 31 commits into from
May 5, 2022

Conversation

mjgrzybek
Copy link
Contributor

@mjgrzybek mjgrzybek commented Apr 26, 2022

Description

Currently, user is expected to install CRD manually. Once it's done, helm charts can be used to instantiate dynakube object containing configuration.

It's about to change:

  • helm chart is responsible only for CRD deployment
  • user must create CR manually
  • user must create token secrets manually

This PR is the first step - helm charts won't create dynakube object.

csi section renamed to csidriver.
Added csi.enabled field.

Added securityContextConstraints section.

How can this be tested?

  1. Install latest stable release:
k create ns dynatrace
kubectl apply -f https://github.com/Dynatrace/dynatrace-operator/releases/latest/download/dynatrace.com_dynakubes.yaml
helm install --namespace dynatrace dynatrace-operator dynatrace/dynatrace-operator -f values-old.yaml --atomic
  1. Upgrade to new helm chart:
    helm upgrade dynatrace-operator config/helm/chart/default/ -f values-new.yaml --atomic --namespace dynatrace

Example input

values-old.yaml

# may be set to "kubernetes", "openshift", "google"
platform: "kubernetes"

### base properties

name: "dynakube"
apiUrl: "..."
image: quay.io/dynatrace/dynatrace-operator:0.5.2

operator:
  requests:
    cpu: 50m
    memory: 64Mi
  limits:
    cpu: 100m
    memory: 128Mi

webhook:
  requests:
    cpu: 300m
    memory: 128Mi
  limits:
    cpu: 300m
    memory: 128Mi
  highAvailability: false

csi:
  requests:
    cpu: 300m
    memory: 100Mi
  limits:
    cpu: 300m
    memory: 100Mi

cloudNativeFullStack:
  enabled: true

values-new.yaml:

platform: "kubernetes"

image: quay.io/dynatrace/dynatrace-operator:snapshot

operator:
  requests:
    cpu: 50m
    memory: 64Mi
  limits:
    cpu: 100m
    memory: 128Mi

webhook:
  requests:
    cpu: 300m
    memory: 128Mi
  limits:
    cpu: 300m
    memory: 128Mi
  highAvailability: false

csi:
  requests:
    cpu: 300m
    memory: 100Mi
  limits:
    cpu: 300m
    memory: 100Mi

csidriver:
  enabled: true

Checklist

  • Unit tests have been updated/added
  • PR is labeled accordingly

@mjgrzybek mjgrzybek added the helm Changes to helm templates or values file label Apr 26, 2022
@mjgrzybek mjgrzybek force-pushed the feature/remove-cr-from-helm-chart branch from 801858f to 37e7af7 Compare April 26, 2022 12:21
@chrismuellner
Copy link
Collaborator

Don't forget to update the NOTES.txt file!

@mjgrzybek mjgrzybek marked this pull request as ready for review April 29, 2022 09:31
@mjgrzybek mjgrzybek requested a review from a team as a code owner April 29, 2022 09:31
@mjgrzybek mjgrzybek force-pushed the feature/remove-cr-from-helm-chart branch from c28eb1f to 32f339f Compare April 29, 2022 09:32
@mjgrzybek mjgrzybek enabled auto-merge (squash) April 29, 2022 10:50
config/deploy/kubernetes/kubernetes-all.yaml Outdated Show resolved Hide resolved
config/helm/chart/default/questions.yml Show resolved Hide resolved
config/helm/chart/default/templates/_helpers.tpl Outdated Show resolved Hide resolved
config/helm/chart/default/values.yaml Outdated Show resolved Hide resolved
config/helm/chart/default/values.yaml Outdated Show resolved Hide resolved
config/helm/chart/default/README.md Outdated Show resolved Hide resolved
config/helm/chart/default/README.md Outdated Show resolved Hide resolved
config/helm/chart/default/README.md Outdated Show resolved Hide resolved
config/helm/chart/default/templates/_helpers.tpl Outdated Show resolved Hide resolved
config/helm/chart/default/values.yaml Outdated Show resolved Hide resolved
@meik99
Copy link
Contributor

meik99 commented May 5, 2022

while testing i noticed that the CREATE operation is not applied to the validation webhook, because the .manifests property is missing. I'd suggest removing that check entirely and adding CREATE to the operations rule permanently. Otherwise the creation of invalid custom resources would be allowed

@mjgrzybek
Copy link
Contributor Author

while testing i noticed that the CREATE operation is not applied to the validation webhook, because the .manifests property is missing. I'd suggest removing that check entirely and adding CREATE to the operations rule permanently. Otherwise the creation of invalid custom resources would be allowed

@meik99 Good point 👍 but it changes webhook behavior which is out of scope of this PR. Let's create another PR with this change separated so we can track results with better granularity.

@meik99
Copy link
Contributor

meik99 commented May 5, 2022

while testing i noticed that the CREATE operation is not applied to the validation webhook, because the .manifests property is missing. I'd suggest removing that check entirely and adding CREATE to the operations rule permanently. Otherwise the creation of invalid custom resources would be allowed

@meik99 Good point +1 but it changes webhook behavior which is out of scope of this PR. Let's create another PR with this change separated so we can track results with better granularity.

It changes webhook behaviour as it is now, because previously the validation webhook caught invalid CRs on creation

@0sewa0
Copy link
Contributor

0sewa0 commented May 5, 2022

Values.manifests is definitely no longer needed
probably Values.partial is also no longer necessary

@mjgrzybek mjgrzybek force-pushed the feature/remove-cr-from-helm-chart branch from 6520602 to 41d5e0f Compare May 5, 2022 06:44
@0sewa0
Copy link
Contributor

0sewa0 commented May 5, 2022

I take it back for Values.partial, that is still kinda needed (unfortunately 😭 )

@mjgrzybek
Copy link
Contributor Author

Values.manifests is definitely no longer needed probably Values.partial is also no longer necessary

@0sewa0 .partial is used to determine if csi is needed src
but I'll gladly change it's name and values ("false" and "csi" :D) to more descriptive.

@0sewa0
Copy link
Contributor

0sewa0 commented May 5, 2022

@mjgrzybek I would leave it as is for now,
as the main idea for not using a bool for the partial thingy is that then it could be used for different things in the future

we should probably move the csi-driver into a sub-chart so it could be easily and neatly generated without these hacky ifs :D (but that is a problem for another time)

@meik99
Copy link
Contributor

meik99 commented May 5, 2022

when autoCreateSecret is true, the upgrade is not possible, as the secret.yaml looks for .Values.name which has now been removed.
I'd suggest either:

  • Re-adding that property in the values file
  • Changing the secret.yaml to use .Release.Name
  • Removing the ability to automatically create the secret

@meik99
Copy link
Contributor

meik99 commented May 5, 2022

The upgrade fails since the operator's readiness and liveness probes fail:

Warning  Unhealthy  2m10s (x6 over 3m10s)  kubelet            Readiness probe failed: HTTP probe failed with statuscode: 404
Warning  Unhealthy  2m10s (x6 over 3m10s)  kubelet            Liveness probe failed: HTTP probe failed with statuscode: 404

@meik99
Copy link
Contributor

meik99 commented May 5, 2022

The upgrade fails since the operator's readiness and liveness probes fail:

Warning  Unhealthy  2m10s (x6 over 3m10s)  kubelet            Readiness probe failed: HTTP probe failed with statuscode: 404
Warning  Unhealthy  2m10s (x6 over 3m10s)  kubelet            Liveness probe failed: HTTP probe failed with statuscode: 404

nevermind, works with the newer images

meik99
meik99 previously approved these changes May 5, 2022
@0sewa0
Copy link
Contributor

0sewa0 commented May 5, 2022

@mjgrzybek run make manifests than it can go in 😄

@0sewa0 0sewa0 disabled auto-merge May 5, 2022 07:56
@mjgrzybek mjgrzybek merged commit e978a71 into master May 5, 2022
@mjgrzybek mjgrzybek deleted the feature/remove-cr-from-helm-chart branch May 5, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm Changes to helm templates or values file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants