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

🌱 Utilize tilt-provider.yaml and Tilt deploy approach from the Cluster API #726

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

dmvolod
Copy link
Member

@dmvolod dmvolod commented Feb 14, 2025

What this PR does / why we need it:
This PR adopts tilt utilizing approach from the Cluster API for deploying and debugging operator.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #719

Some issue need to be fixed in Cluster API Tiltfile to avoid Cluster API Core Provider bootstrapping together with the operator

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 14, 2025
Copy link

netlify bot commented Feb 14, 2025

Deploy Preview for kubernetes-sigs-cluster-api-operator ready!

Name Link
🔨 Latest commit bb2b9a1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-operator/deploys/67bd8b2670fba50008f9fafb
😎 Deploy Preview https://deploy-preview-726--kubernetes-sigs-cluster-api-operator.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 20, 2025

/hold

Will continue, when kubernetes-sigs/cluster-api#11879 will merged

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2025
@dmvolod dmvolod marked this pull request as ready for review February 20, 2025 10:09
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2025
@furkatgofurov7
Copy link
Member

furkatgofurov7 commented Feb 21, 2025

@dmvolod is it still on hold? FYI, we are planning a release early next week and this could well be part of it.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 21, 2025

@dmvolod is it still on hold? FYI, we are planning a release early next week and this could well be part of it.

Yes, @furkatgofurov7 but this is almost done, I need to fix just documentation. Will do it soon.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 21, 2025

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2025
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Thanks for adding documentation, nice!
I have few suggestions inline that hopefully makes sense and improves the wording:

@dmvolod
Copy link
Member Author

dmvolod commented Feb 24, 2025

Thanks for adding documentation, nice! I have few suggestions inline that hopefully makes sense and improves the wording:

Thanks @furkatgofurov7 for the review and feedback provided

@furkatgofurov7
Copy link
Member

I was not able to successfully run tilt locally (but also have not had time to debug it further) with these changes:

 → Creating ClusterRoleBinding="cert-manager-controller-certificatesigningrequests"
 → Creating ClusterRoleBinding="cert-manager-webhook:subjectaccessreviews"
 → Creating Role="cert-manager-cainjector:leaderelection" Namespace="kube-system"
 → Creating Role="cert-manager:leaderelection" Namespace="kube-system"
 → Creating Role="cert-manager-tokenrequest" Namespace="cert-manager"
 → Creating Role="cert-manager-webhook:dynamic-serving" Namespace="cert-manager"
 → I0224 11:50:48.667940   11580 main.go:505] [resources/cert-manager-controller] task completed, elapsed: 1.675950709s
 → I0224 11:50:48.667960   11580 main.go:453] [resources] task group completed, elapsed: 1.67614025s
 → F0224 11:50:48.667971   11580 main.go:194] [main] failed to prepare tilt resources: [resources/cert-manager-controller] failed to run [kind load docker-image --name=capi-test quay.io/jetstack/cert-manager-controller:v1.16.3]: ERROR: image: "quay.io/jetstack/cert-manager-controller:v1.16.3" not present locally
 → : exit status 1
Traceback (most recent call last):
  /Users/user/repos/testing-locally/cluster-api/Tiltfile:645:12: in <toplevel>
  /Users/user/repos/testing-locally/cluster-api/Tiltfile:499:10: in prepare_all
Error in local: command "make -B tilt-prepare && ./hack/tools/bin/tilt-prepare --tools kustomize,clusterctl --tilt-settings-file ./tilt-settings.yaml" failed.
error: exit status 1

also another concern is: this creates a soft dependency to CAPI scripts and Tiltfile, meaning we will be dependant and should accept whatever changes proposed to upstream in regards to local development. Can't we just enable debugger using the current Tiltfile in the repo?

@dmvolod
Copy link
Member Author

dmvolod commented Feb 24, 2025

also another concern is: this creates a soft dependency to CAPI scripts and Tiltfile, meaning we will be dependant and should accept whatever changes proposed to upstream in regards to local development. Can't we just enable debugger using the current Tiltfile in the repo?

Will look into the issue with clean environment, but seem to this is output from MacOS, but I'm using Linux
Not sure, this is very easy and good approach is to copy debug setup code from Cluster API env or create it from scratch. The main concern is reusing Cluster API experience and be in sync.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 24, 2025

F0224 11:50:48.667971 11580 main.go:194] [main] failed to prepare tilt resources: [resources/cert-manager-controller] failed to run [kind load docker-image --name=capi-test quay.io/jetstack/cert-manager-controller:v1.16.3]: ERROR: image: "quay.io/jetstack/cert-manager-controller:v1.16.3" not present locally

It seems to this is OS or tilt version or tilt:extension problem. Please verify that are you using latest tilt binary and latest main code from the Cluster API.

@furkatgofurov7
Copy link
Member

F0224 11:50:48.667971 11580 main.go:194] [main] failed to prepare tilt resources: [resources/cert-manager-controller] failed to run [kind load docker-image --name=capi-test quay.io/jetstack/cert-manager-controller:v1.16.3]: ERROR: image: "quay.io/jetstack/cert-manager-controller:v1.16.3" not present locally

It seems to this is OS or tilt version or tilt:extension problem. Please verify that are you using latest tilt binary and latest main code from the Cluster API.

I am on MacOS and had to pull cert-manager images locally&load them into kind cluster, and it was running fine.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 25, 2025

F0224 11:50:48.667971 11580 main.go:194] [main] failed to prepare tilt resources: [resources/cert-manager-controller] failed to run [kind load docker-image --name=capi-test quay.io/jetstack/cert-manager-controller:v1.16.3]: ERROR: image: "quay.io/jetstack/cert-manager-controller:v1.16.3" not present locally

It seems to this is OS or tilt version or tilt:extension problem. Please verify that are you using latest tilt binary and latest main code from the Cluster API.

I am on MacOS and had to pull cert-manager images locally&load them into kind cluster, and it was running fine.

It seems to should work out-of-the box in Cluster API for both Linux and MacOS.
I can submit this issue to the Cluster API, but unfortunately can't debug and fix it, as don't have MacOS for experiments.

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

After giving it another thought, this could be a good improvement and provides more options to users developing locally through the CAPI exposed tilt-settings fields.

/approve
cc @Danil-Grigorev @alexander-demicev

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2025
@furkatgofurov7
Copy link
Member

It seems to should work out-of-the box in Cluster API for both Linux and MacOS.
I can submit this issue to the Cluster API, but unfortunately can't debug and fix it, as don't have MacOS for experiments.

There should not be an issue with CAPI, it just expects all images to be pre-pulled before running it (they should even have prepare-env or similar makefile target that pulls all necessary images), so all good.

Copy link
Member

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

Thank you, that seems like a very good simplification for the dev environment setup. I left one suggestion for building CAPI images and using in this setup.

@dmvolod
Copy link
Member Author

dmvolod commented Feb 25, 2025

It seems to should work out-of-the box in Cluster API for both Linux and MacOS.
I can submit this issue to the Cluster API, but unfortunately can't debug and fix it, as don't have MacOS for experiments.

There should not be an issue with CAPI, it just expects all images to be pre-pulled before running it (they should even have prepare-env or similar makefile target that pulls all necessary images), so all good.

On Linux it's not needed to pre-load images manually or with separate target
Please have a look on the test for Linux:

12:10 $ docker images | grep quay.io/jetstack
✘-1 /u01/git/kubernetes-sigs/cluster-api [main|✔] 
12:10 $ KIND_NETWORK_IPFAMILY=ipv4 make tilt-up
hack/kind-install-for-capd.sh
No kind clusters found.
Creating cluster "capi-test" ...
 ✓ Ensuring node image (kindest/node:v1.32.2) 🖼
 ✓ Preparing nodes 📦  
 ✓ Writing configuration 📜 
 ✓ Starting control-plane 🕹️ 
 ✓ Installing CNI 🔌 
 ✓ Installing StorageClass 💾 
Set kubectl context to "kind-capi-test"
You can now use your cluster with:

kubectl cluster-info --context kind-capi-test

Thanks for using kind! 😊
configmap/local-registry-hosting created
tilt up
Tilt started on http://localhost:10350/
v0.33.22, built 2025-01-03

(space) to open the browser
(s) to stream logs (--stream=true)
(t) to open legacy terminal mode (--legacy=true)
(ctrl-c) to exit
Opening browser: http://localhost:10350/
Tilt started on http://localhost:10350/
v0.33.22, built 2025-01-03
.
.
.
12:12 $ docker images | grep quay.io/jetstack
quay.io/jetstack/cert-manager-webhook                                  v1.16.3                 d995a2979670   5 weeks ago     61.4MB
quay.io/jetstack/cert-manager-controller                               v1.16.3                 27ebcc620bde   5 weeks ago     72.4MB
quay.io/jetstack/cert-manager-cainjector                               v1.16.3                 ce7242a2b54b   5 weeks ago     52.8MB

@furkatgofurov7
Copy link
Member

furkatgofurov7 commented Feb 25, 2025

On Linux it's not needed to pre-load images manually or with separate target
Please have a look on the test for Linux:

@dmvolod that is probably because you had them pulled (5 weeks ago as per logs) locally already, but in my case it was not. It might be also, MacOS has a problem to load local images into kind cluster unless it is explicitly called out by kind command in the script.

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 09462d3151e4063172e4d2ce34fab0d0b638fbab

@dmvolod
Copy link
Member Author

dmvolod commented Feb 25, 2025

On Linux it's not needed to pre-load images manually or with separate target
Please have a look on the test for Linux:

@dmvolod that is probably because you had them pulled (5 weeks ago as per logs) locally already, but in my case it was not. It might be also, MacOS has a problem to load local images into kind cluster unless it is explicitly called out by kind command in the script.

@furkatgofurov7 seems to this is MacOS issue, for my env images were pulled automatically while Tiltfile running
CREATED is not local value, it's a data from the docker file itself

12:32 $ docker inspect d995a2979670 | grep -i created
        "Created": "2025-01-16T11:05:57.181706423Z"

@k8s-ci-robot k8s-ci-robot merged commit da897f7 into kubernetes-sigs:main Feb 25, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add debug option to Tiltfile
4 participants