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

Add Phase Skipping for Kubeadm & Relase #10

Merged
merged 9 commits into from
Jul 18, 2024

Conversation

drew-viles
Copy link
Contributor

  • Updated the charts to all latest available versions
  • Updated some docs with typo fixes and added some additional exmaples.
  • Added ability to skip kube-proxy so that cilium can take over 🥳

I've deployed these manually both with and without the kube-proxy skip phase enabled and both times it's worked as expected.

Copy link
Member

@spjmurray spjmurray left a comment

Choose a reason for hiding this comment

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

Perhaps you want to bump the version in the cluster chart so it actually gets released?

charts/cluster-api-cluster-openstack/values.yaml Outdated Show resolved Hide resolved
charts/cluster-api-cluster-openstack/values.yaml Outdated Show resolved Hide resolved
@yankcrime
Copy link
Member

I think I'd prefer this as two changes, one for the CAPI and CAPO updates and a second for the Cilium / kube-proxy changes. I also think that this should be the new default, so instead of introducing anything optional it's just what we do from now on.

@drew-viles
Copy link
Contributor Author

I think I'd prefer this as two changes, one for the CAPI and CAPO updates and a second for the Cilium / kube-proxy changes. I also think that this should be the new default, so instead of introducing anything optional it's just what we do from now on.

I'm more than happy to split it out. It would make sense really in the scheme of things.

@drew-viles
Copy link
Contributor Author

Perhaps you want to bump the version in the cluster chart so it actually gets released?

Will do. I wasn't sure if you wanted to do a release separately so wanted to ask about that, but you're too quick for me 🤣

README.md Outdated Show resolved Hide resolved
@drew-viles drew-viles mentioned this pull request Jul 17, 2024
@spjmurray
Copy link
Member

What "we" want and what the rest of the world want may be different things... plus you are changing the default behaviour in what may be a breaking way. I'd personally prefer we play it safe.

@yankcrime
Copy link
Member

The rest of the world are free to fork, such is the beauty of open source.

I understand the aversion for such a fundamental change however, so let's keep the existing as the default until we're happy we've done sufficient testing to have the confidence it should be switched.

@spjmurray
Copy link
Member

It is a fair compromise. I'm figuring that said change would require some form of upgrade instructions tell us why the change is being made in the first place (performance? stability? paying Cisco 😸 ), and any other changes that need to be made in order to make it work, I'm guessing cilium is going to collapse in a heap if you just upgrade blindly?

@drew-viles
Copy link
Contributor Author

@spjmurray @yankcrime There we go, much cleaner :-)

@drew-viles
Copy link
Contributor Author

Rebased off of main so I'll request reviews again as a matter of course @spjmurray @yankcrime

@drew-viles drew-viles requested a review from yankcrime July 18, 2024 11:28
Copy link
Member

@spjmurray spjmurray left a comment

Choose a reason for hiding this comment

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

You may want to update the patch version in charts/cluster-api-cluster-openstack/Chart.yaml to release a new version, unless you want to wait for a follow-up?

charts/cluster-api-cluster-openstack/values.yaml Outdated Show resolved Hide resolved
@drew-viles drew-viles requested a review from spjmurray July 18, 2024 12:12
@drew-viles drew-viles force-pushed the update-docs branch 2 times, most recently from 183afc2 to 6efb94e Compare July 18, 2024 12:18
@drew-viles drew-viles changed the title Add skip-kube-proxy & chart updates Add skipPhases for kubeadm & chart updates Jul 18, 2024
@drew-viles
Copy link
Contributor Author

I'm gonna rebase again too.

* option to skip kube-proxy phase of kubeadm init - defaults to false
* option to register the node - defaults to true
…ugh and also added more to the comment explaining what it is.
@drew-viles
Copy link
Contributor Author

ack, I'm not sure why it's failing on the git status --porcelain test?

@drew-viles
Copy link
Contributor Author

drew-viles commented Jul 18, 2024

ok so it looks like it's the tests you've implemented :-p @spjmurray

if I don't run make charts, make docs, it fails in the pipeline because it will update those due to the tag change in the Makefile.
If I do run it, it fails at the make charts because the charts don't exist yet in the Helm repo.

The only way I can get this to pass is to not do a chart tag update in the Makefile on this run because the make charts will pull the version from the helm repo - which isn't really testing these changes.

We might need an additional make target which can run against the path rather than the helm repo for pipeline testing?

@spjmurray
Copy link
Member

@drew-viles than you for being my guinea pig 😸

@spjmurray
Copy link
Member

So what you need to do is apply this:

[Thu 18 Jul 14:00:26 BST 2024] simon@symphony ~/src/github.com/unikorn-cloud/helm-cluster-api git diff
diff --git a/Makefile b/Makefile
index 33d639e..af1528f 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
 # Update this for every tagged release.
-CHART_VERSION = v0.2.0
+CHART_VERSION = v0.2.1
 
 # Defines the versions to use for cluster API components.
 CAPI_VERSION = v1.7.4
diff --git a/charts/cluster-api-cluster-openstack/Chart.yaml b/charts/cluster-api-cluster-openstack/Chart.yaml
index 8fe85a3..c958a54 100644
--- a/charts/cluster-api-cluster-openstack/Chart.yaml
+++ b/charts/cluster-api-cluster-openstack/Chart.yaml
@@ -2,5 +2,5 @@ apiVersion: v2
 name: cluster-api-cluster-openstack
 description: A Helm chart to deploy a Kubernetes Cluster
 type: application
-version: v0.4.4
+version: v0.4.5
 icon: https://raw.githubusercontent.com/unikorn-cloud/helm-cluster-api/main/icons/default.png

then run make && make docs && git commit -a --amend

@drew-viles
Copy link
Contributor Author

Boom! There ya go @spjmurray Nice one.

@drew-viles
Copy link
Contributor Author

aaah poop - no dice! :-D Trigger happy on the comment :-p

@spjmurray
Copy link
Member

in theory anyway!

@spjmurray spjmurray changed the title Add skipPhases for kubeadm & chart updates Add Phase Skipping for Kubeadm & Relase Jul 18, 2024
@spjmurray
Copy link
Member

I see, lemme update the script to correctly generate the cluster-api chart....

@spjmurray
Copy link
Member

diff --git a/Makefile b/Makefile
index 33d639e..1f6488b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,5 +1,5 @@
 # Update this for every tagged release.
-CHART_VERSION = v0.2.0
+CHART_VERSION = v0.2.1
 
 # Defines the versions to use for cluster API components.
 CAPI_VERSION = v1.7.4
@@ -24,7 +24,7 @@ all: cluster-api
 
 .PHONY: cluster-api
 cluster-api: $(CHARTS)
-       ./generate-capi.py $(CHARTS)
+       ./generate-capi.py -v $(CHART_VERSION) -c $(CAPI_VERSION) $(CHARTS)
 
 .PHONY: cluster-api-core
 cluster-api-core:

and...

diff --git a/generate-capi.py b/generate-capi.py
index eba18e6..c1273a8 100755
--- a/generate-capi.py
+++ b/generate-capi.py
@@ -1,14 +1,23 @@
 #!/usr/bin/env python3
 
+import argparse
+import re
 import sys
 
 def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument('-v', '--version', help="Chart version")
+    parser.add_argument('-c', '--capi-version', help='Cluster API version')
+    parser.add_argument('subcharts', nargs='+', help="Subcharts to include")
+
+    args = parser.parse_args()
+
     chunks = []
 
     with open('charts/cluster-api/values.yaml.tmpl') as file:
         chunks.append(file.read())
 
-    for chart in sys.argv[1:]:
+    for chart in args.subcharts:
         chunk = [f"{chart}:\n"]
 
         with open(f'charts/{chart}/values.yaml') as file:
@@ -19,5 +28,25 @@ def main():
     with open('charts/cluster-api/values.yaml', 'w') as file:
         file.write('\n'.join(chunks))
 
+    with open(f'charts/cluster-api/Chart.yaml', 'w') as file:
+        file.write("apiVersion: v2\n")
+        file.write(f"appVersion: {args.capi_version}\n")
+        file.write("name: cluster-api\n")
+        file.write("description: A Helm chart to deploy Cluster API\n")
+        file.write("type: application\n")
+        file.write(f"version: {args.version}\n")
+        file.write("icon: https://assets.unikorn-cloud.org/assets/images/logos/dark-on-light/icon.png\n")
+        file.write("\n")
+        file.write("dependencies:\n")
+
+        for chart in args.subcharts:
+            file.write(f"- name: {chart}\n")
+            file.write(f"  version: {args.version}\n")
+            file.write(f"  repository: file://../{chart}\n")
+            # Oh for want of a better way of doing this...
+            matches = re.match(r'^cluster-api-(?:bootstrap|control-plane|provider)-(.*)', chart)
+            if matches:
+                file.write(f"  condition: {matches[1]}.enabled\n")
+
 if __name__ == '__main__':
     main()

@spjmurray
Copy link
Member

Then run make && make docs again obvs

@drew-viles
Copy link
Contributor Author

Nicely done @spjmurray All passed!

@spjmurray spjmurray merged commit ec49678 into unikorn-cloud:main Jul 18, 2024
1 check passed
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.

3 participants