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

Sandbox debugging #89

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Conversation

liamfallon
Copy link
Member

@liamfallon liamfallon commented Jul 26, 2024

This PR tweaks the launch configuration and Makefile to allow remote debugging of Porch on a Nephio sandbox VM.

This tweak requires explicit inventory parameters to be used, see nephio-project/test-infra#293

The documentation for remote VM debugging is in this PR: nephio-project/docs#158

@nephio-prow nephio-prow bot requested review from henderiw and s3wong July 26, 2024 15:26
@nephio-prow nephio-prow bot added the approved label Jul 26, 2024
@liamfallon
Copy link
Member Author

/assign @efiacor @kispaljr @arora-sagar

@liamfallon
Copy link
Member Author

I couldn't get these Make targets to work with kpt as @kispaljr had done in the original run-in-kind targets so I used kubectl directly instead. I think this is because the Makefile is not in control of the kpt package used to deploy Porch in the sandbox. I admit that I did not go to extremes in trying to get kpt to work.

Also, this PR works on the assumption that the Porch package installed in the sandbox VM is identical to the package under development (CRDs etc) except for what's in the pod deployments. I think that's a fair assumption.

@kispaljr
Copy link
Collaborator

kispaljr commented Jul 26, 2024

I've read the VM setup documentation that you linked in the PR, but I still do not understand what "default" and "override" deployments are.
@liamfallon could you elaborate on the new make targets a bit, please?

"--secure-port=4443",
"--kubeconfig=${userHome}/.kube/config",
"--cache-directory=${workspaceFolder}/.cache",
"--function-runner=172.18.0.202:9445",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the IP of the function-runner the only difference between "Launch Server" and "Launch Override Server"? If yes, could we avoid copy-pasting here by using e.g. environment variables?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or even better can we use the same IP in both setups? What prevents this?

Choose a reason for hiding this comment

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

I would vote for the environmental variable approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixed in the latest PR.

Makefile Outdated
@@ -256,6 +257,26 @@ run-in-kind-no-controller: IMAGE_TAG=test
run-in-kind-no-controller: SKIP_CONTROLLER_BUILD=true
run-in-kind-no-controller: load-images-to-kind deployment-config-no-controller deploy-current-config ## Build and deploy porch without the controllers into a kind cluster

.PHONY: run-default
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we achieve the same effect by something similar to the following process:

  • add these lines to the bashrc of the sandbox VM:
export DEPLOYPORCHCONFIGDIR=/tmp/kpt-pkg/nephio/core/porch
export KIND_CONTEXT_NAME=<whatever is the name of the kind cluster in the Nephio sandbox :)>
  • reuse the existing make targets: run-in-kind-*

Copy link
Collaborator

Choose a reason for hiding this comment

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

but the simplest solution IMO is to undeploy the porch instance that was deployed by the ansible playbook (call kpt live destroy /tmp/kpt-pkg/nephio/core/porch) and then reinstall it with make run-in-kind. and then use the same development process that is currently documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

but the simplest solution IMO is to undeploy the porch instance that was deployed by the ansible playbook (call kpt live destroy /tmp/kpt-pkg/nephio/core/porch) and then reinstall it with make run-in-kind. and then use the same development process that is currently documented.

The problem there is that if you uninstall, it undefines all the package variants and packagerevisions that are put in place by the Nephio install.

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't we achieve the same effect by something similar to the following process:

* add these lines to the bashrc of the sandbox VM:
export DEPLOYPORCHCONFIGDIR=/tmp/kpt-pkg/nephio/core/porch
export KIND_CONTEXT_NAME=<whatever is the name of the kind cluster in the Nephio sandbox :)>
* reuse the existing make targets: `run-in-kind-*`

The problem I had was that I couldn't get kpt to play nicely with the old and new package. I tried to use the existing targets but because the underlying kpt wouldn't work, they failed. Maybe you could take a look @kispaljr

Copy link
Collaborator

Choose a reason for hiding this comment

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

but the simplest solution IMO is to undeploy the porch instance that was deployed by the ansible playbook (call kpt live destroy /tmp/kpt-pkg/nephio/core/porch) and then reinstall it with make run-in-kind. and then use the same development process that is currently documented.

The problem there is that if you uninstall, it undefines all the package variants and packagerevisions that are put in place by the Nephio install.

OK, I will try this out

Choose a reason for hiding this comment

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

By the way a tiny opinion, is it possible to use _ like DEFAULT_PORCH_CONFIG_DIR it increases the readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is all changed int he new version of the PR.

@liamfallon
Copy link
Member Author

I've read the VM setup documentation that you linked in the PR, but I still do not understand what "default" and "override" deployments are. @liamfallon could you elaborate on the new make targets a bit, please?

The "default" install is the install that is already on the VM when it is installed by the nephio init script. In the sandbox, it is in /tmp/kpt-pkg/nephio/core/porch so it's the default install for Porch in Nephio.

The "override" one is the development version that we are installing, building, and debugging ont he vm, so it overrides the default install.

Maybe the names I picked are not great.

@liamfallon
Copy link
Member Author

/test presubmit-nephio-go-test

@liamfallon
Copy link
Member Author

I've read the VM setup documentation that you linked in the PR, but I still do not understand what "default" and "override" deployments are. @liamfallon could you elaborate on the new make targets a bit, please?

They are removed in the new version of the PR.

@liamfallon
Copy link
Member Author

/test presubmit-nephio-go-test

1 similar comment
@liamfallon
Copy link
Member Author

/test presubmit-nephio-go-test

@liamfallon
Copy link
Member Author

/assign @efiacor

@arora-sagar
Copy link

I tried it works thank you @liamfallon

@efiacor
Copy link
Collaborator

efiacor commented Aug 7, 2024

/approve

Copy link
Contributor

nephio-prow bot commented Aug 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: efiacor, liamfallon

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

@kispaljr
Copy link
Collaborator

kispaljr commented Aug 7, 2024

/lgtm

@nephio-prow nephio-prow bot added the lgtm label Aug 7, 2024
@nephio-prow nephio-prow bot merged commit ecf97ef into nephio-project:main Aug 7, 2024
5 checks passed
nephio-prow bot pushed a commit to nephio-project/docs that referenced this pull request Aug 8, 2024
@liamfallon liamfallon deleted the sandbox-debugging branch August 29, 2024 13:50
@liamfallon liamfallon restored the sandbox-debugging branch August 29, 2024 13:50
@liamfallon liamfallon deleted the sandbox-debugging branch August 29, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants