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

install: Deploy peerpod-ctrl by default #1665

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

bpradipt
Copy link
Member

The default value of RESOURCE_CTRL is set to true to allow deletion of dangling resources.

The default value of RESOURCE_CTRL is set to true to allow deletion
of dangling resources.

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Copy link
Contributor

@huoqifeng huoqifeng left a comment

Choose a reason for hiding this comment

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

LGTM

@huoqifeng
Copy link
Contributor

Thank you @bpradipt

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Do we need to some something to make the e2e tests also deploy the peerpod-ctrl?
Maybe after

log.Info("Install the cloud-api-adaptor")
if err := p.installOverlay.Apply(ctx, cfg); err != nil {
return err
}
? I want to avoid us having different default config in the CI tests from the other deployment approach(es).

Maybe we should add a line to the manual install path of the readme as well to tie up those loose ends?

@bpradipt
Copy link
Member Author

Do we need to some something to make the e2e tests also deploy the peerpod-ctrl? Maybe after

log.Info("Install the cloud-api-adaptor")
if err := p.installOverlay.Apply(ctx, cfg); err != nil {
return err
}

? I want to avoid us having different default config in the CI tests from the other deployment approach(es).
Maybe we should add a line to the manual install path of the readme as well to tie up those loose ends?

Yeah good point. I'll look into how to enable it in e2e by default

Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

The provisioning update code looks okay to me. Sorry to be a pain, but should we consider running peerpod-ctrl undeploy somewhere around

ccPods, err := GetDaemonSetOwnedPods(ctx, cfg, p.ccDaemonSet)
?

@bpradipt bpradipt force-pushed the peerpod-ctrl-def branch 2 times, most recently from 7b533ba to fe59a64 Compare January 18, 2024 11:09
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Jan 18, 2024
Deploy peerpod-ctrl by default as part of e2e test

Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Code LGTM, so as long as the e2e libvirt tests pass, I'm happy for this to be merged. Thanks @bpradipt for the code and updates.

@stevenhorsman
Copy link
Member

The e2e tests are failing with:

Traceback (most recent call last):
  File "/usr/bin/kcli", line 33, in <module>
    sys.exit(load_entry_point('kcli==99.0', 'console_scripts', 'kcli')())
  File "/usr/lib/python3/dist-packages/kvirt/cli.py", line 5416, in cli
    args.func(args)
  File "/usr/lib/python3/dist-packages/kvirt/cli.py", line 131, in get_version
    git_version, git_date = get_git_version()
  File "/usr/lib/python3/dist-packages/kvirt/common/__init__.py", line [21](https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/7570170726/job/20616589992?pr=1665#step:10:22)14, in get_git_version
    git_version, git_date = open(git_file).read().rstrip().split(' ')

this looks like the same issue that @karmab fixed a couple of weeks ago, though 😕

@wainersm
Copy link
Member

hi @bpradipt !

In order to test peerpod-ctrl changes on pull request you will need to build and push its image like we do for podvm and cloud-api-adaptor images. Otherwise it will be testing quay.io/confidential-containers/peerpod-ctrl:latest, that is published on every merge.

@karmab
Copy link

karmab commented Jan 18, 2024

The e2e tests are failing with:

Traceback (most recent call last):
  File "/usr/bin/kcli", line 33, in <module>
    sys.exit(load_entry_point('kcli==99.0', 'console_scripts', 'kcli')())
  File "/usr/lib/python3/dist-packages/kvirt/cli.py", line 5416, in cli
    args.func(args)
  File "/usr/lib/python3/dist-packages/kvirt/cli.py", line 131, in get_version
    git_version, git_date = get_git_version()
  File "/usr/lib/python3/dist-packages/kvirt/common/__init__.py", line [21](https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/7570170726/job/20616589992?pr=1665#step:10:22)14, in get_git_version
    git_version, git_date = open(git_file).read().rstrip().split(' ')

this looks like the same issue that @karmab fixed a couple of weeks ago, though 😕

let me check

@bpradipt
Copy link
Member Author

hi @bpradipt !

In order to test peerpod-ctrl changes on pull request you will need to build and push its image like we do for podvm and cloud-api-adaptor images. Otherwise it will be testing quay.io/confidential-containers/peerpod-ctrl:latest, that is published on every merge.

@wainersm that's not the intent and frankly out of scope for this PR. peerpod-ctrl is treated like the operator here.
Sometime back we had a discussion to move out peerpod-ctrl and peerpodconfig-ctrl to it's own repos to make it easier for testing.

@wainersm
Copy link
Member

hi @bpradipt !
In order to test peerpod-ctrl changes on pull request you will need to build and push its image like we do for podvm and cloud-api-adaptor images. Otherwise it will be testing quay.io/confidential-containers/peerpod-ctrl:latest, that is published on every merge.

@wainersm that's not the intent and frankly out of scope for this PR. peerpod-ctrl is treated like the operator here. Sometime back we had a discussion to move out peerpod-ctrl and peerpodconfig-ctrl to it's own repos to make it easier for testing.

Alright, thanks for the explanation!

@karmab
Copy link

karmab commented Jan 18, 2024

the issue is that github change again their commits page format (with no mention nowhere...), so I fixed it with https://github.com/karmab/kcli/actions/runs/7573926453 so that

  • git ls-remote is used to populate the version instead
  • if the version file is incorrectly formatted, no exception is thrown

@wainersm wainersm added test_e2e_libvirt Run Libvirt e2e tests and removed test_e2e_libvirt Run Libvirt e2e tests labels Jan 18, 2024
@bpradipt bpradipt merged commit 8f0abea into confidential-containers:main Jan 19, 2024
37 of 38 checks passed
@bpradipt bpradipt deleted the peerpod-ctrl-def branch January 19, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants