Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Ensures kapp-controller deployment has last-applied annotation before upgrade #3700

Conversation

haoranleo
Copy link
Contributor

@haoranleo haoranleo commented Oct 19, 2022

What this PR does / why we need it

  • Ensures kapp-controller deployment has last-applied annotation before upgrade when package-based-lcm feature flag is on.
  • Same fix as Adds last-applied annotation to kapp-controller deployment before upgrade #3631 but this one is for flow when package-based-lcm feature flag is on. Context about why we need the fix can be referred at PR above.
  • Skips adding last-applied annotation to kapp-controller if it already has one

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  1. Manually verified that creating Classy Cluster based management cluster is successful on AWS.
  2. Verified on TKG cluster on AWS using the tanzu CLI built from this branch, and kapp-controller is upgraded successfully when package-based-lcm feature flag is on:
  • Before upgrade, kapp-controller yaml snippet:
[~] kubectl get deploy kapp-controller -n tkg-system -o yaml
...
metadata:
  annotations:
    kapp-controller.carvel.dev/version: v0.38.4
...
spec:
  template:
    spec:
      containers:
      - command:
        - /kapp-controller-sidecarexec
        env:
        - name: KAPPCTRL_SIDECAREXEC_SOCK
          value: /etc/kappctrl-mem-tmp/sidecarexec.sock
  • Upgrade management cluster:
[~] tanzu config set features.global.package-based-lcm-beta true
[~] tanzu mc upgrade
  • kapp-controller is upgraded successfully:
[~] kubectl get deploy kapp-controller -n tkg-system
NAME              READY   UP-TO-DATE   AVAILABLE   AGE
kapp-controller   1/1     1            1           4m
[~] kubectl get pod -n tkg-system
NAME                                                     READY   STATUS    RESTARTS      AGE
kapp-controller-8494db94cc-xsptn                         2/2     Running   0             40s
tanzu-addons-controller-manager-6f7bd84c87-rcnj5         1/1     Running   0             5d18h
tanzu-capabilities-controller-manager-858569d99c-6k992   1/1     Running   3 (83s ago)   7m47s
tanzu-featuregates-controller-manager-6cc7c6c496-h9vnj   1/1     Running   0             7m27s
[~] kubectl get deploy kapp-controller -n tkg-system -o yaml
...
metadata:
  annotations:
    kapp-controller.carvel.dev/version: v0.41.2
...
spec:
  template:
    spec:
      containers:
      - args:
        - --sidecarexec
        env:
        - name: KAPPCTRL_SIDECAREXEC_SOCK
           value: /etc/kappctrl-mem-tmp/sidecarexec.sock

Release note

Fix kapp-controller upgrade issue when package-based-lcm feature flag is on

Additional information

Special notes for your reviewer

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221019040723/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@haoranleo haoranleo force-pushed the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch from faf78e7 to 4e5569a Compare October 19, 2022 18:30
@haoranleo haoranleo changed the title Ensures kapp-controller deployment has last-applied annotation before upgrade WIP: Ensures kapp-controller deployment has last-applied annotation before upgrade Oct 19, 2022
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221019183632/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #3700 (2754490) into main (b677cd8) will decrease coverage by 0.84%.
The diff coverage is 57.69%.

@@            Coverage Diff             @@
##             main    #3700      +/-   ##
==========================================
- Coverage   46.23%   45.39%   -0.85%     
==========================================
  Files         400      425      +25     
  Lines       39620    41211    +1591     
==========================================
+ Hits        18319    18706     +387     
- Misses      19615    20803    +1188     
- Partials     1686     1702      +16     
Impacted Files Coverage Δ
tkg/client/delete_region.go 0.00% <0.00%> (ø)
tkg/client/init.go 0.00% <0.00%> (ø)
tkg/client/management_components.go 11.96% <0.00%> (ø)
tkg/client/upgrade_region.go 25.44% <0.00%> (ø)
tkg/clusterclient/clusterclient.go 48.71% <25.00%> (+0.40%) ⬆️
...nagementcomponents/management_component_install.go 74.45% <100.00%> (+0.76%) ⬆️
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@haoranleo haoranleo force-pushed the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch from 4e5569a to c3af0c4 Compare October 20, 2022 16:44
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221020165321/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@haoranleo haoranleo force-pushed the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch from c3af0c4 to a95a414 Compare October 20, 2022 20:41
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221020205111/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

… upgrade when package-based-lcm feature is on
@haoranleo haoranleo force-pushed the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch from a95a414 to dfe34ac Compare October 20, 2022 21:59
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221020220819/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@haoranleo haoranleo changed the title WIP: Ensures kapp-controller deployment has last-applied annotation before upgrade Ensures kapp-controller deployment has last-applied annotation before upgrade Oct 20, 2022
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221021220642/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@maralavi maralavi force-pushed the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch from bf7ec0b to 2754490 Compare October 22, 2022 00:23
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3700/20221022003004/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@shyaamsn
Copy link
Contributor

LGTM

@maralavi maralavi added the ok-to-merge PRs should be labelled with this before merging label Oct 24, 2022
@maralavi maralavi merged commit 6d6217c into main Oct 24, 2022
@maralavi maralavi deleted the lhaoran/add-last-applied-annotation-to-kapp-controller-logic-when-feature-flag-is-on branch October 24, 2022 17:40
m1zzx2 pushed a commit that referenced this pull request Oct 25, 2022
… upgrade (#3700)

* Ensures kapp-controller deployment has last-applied annotation before upgrade when package-based-lcm feature is on

* add last-applied annotation conditional on operation type
m1zzx2 pushed a commit that referenced this pull request Oct 25, 2022
… upgrade (#3700)

* Ensures kapp-controller deployment has last-applied annotation before upgrade when package-based-lcm feature is on

* add last-applied annotation conditional on operation type
m1zzx2 pushed a commit that referenced this pull request Oct 31, 2022
… upgrade (#3700)

* Ensures kapp-controller deployment has last-applied annotation before upgrade when package-based-lcm feature is on

* add last-applied annotation conditional on operation type
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants