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.

Sign up for GitHub

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

Jump to bottom

fix(support-bundle): agent doesn't respect node-selector and taint-toleration #1798

Merged
merged 5 commits into from
Apr 8, 2023
Merged

fix(support-bundle): agent doesn't respect node-selector and taint-toleration #1798

merged 5 commits into from
Apr 8, 2023

Conversation

c3y1huang
Copy link
Contributor

@c3y1huang c3y1huang self-assigned this Mar 22, 2023
@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch 6 times, most recently from 32994ae to 5251c1b Compare March 23, 2023 03:15
@innobead
Copy link
Member

innobead commented Apr 6, 2023

@c3y1huang rancher/support-bundle-kit#68 merged. Please create a new image and update this PR.

@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch from 5251c1b to 6839096 Compare April 6, 2023 08:13
@c3y1huang c3y1huang marked this pull request as ready for review April 6, 2023 08:42
@c3y1huang c3y1huang requested a review from a team as a code owner April 6, 2023 08:42
@c3y1huang
Copy link
Contributor Author

c3y1huang commented Apr 6, 2023 •
edited
Loading

@c3y1huang rancher/support-bundle-kit#68 merged. Please create a new image and update this PR.

Waiting for longhornio mirrored image. cc @yangchiu Done

Local test results:

>  ./run.sh -xs -m support_bundle
========================================== test session starts ==========================================
platform linux -- Python 3.9.15, pytest-5.3.1, py-1.11.0, pluggy-0.13.1 -- /usr/bin/python3.9
cachedir: .pytest_cache
rootdir: /integration, inifile: pytest.ini
plugins: order-1.0.1, repeat-0.9.1
collected 387 items / 380 deselected / 7 selected                                                       

test_support_bundle.py::test_support_bundle_should_delete_after_download PASSED
test_support_bundle.py::test_support_bundle_should_error_when_failed PASSED
test_support_bundle.py::test_support_bundle_failed_limit PASSED
test_support_bundle.py::test_support_bundle_purge PASSED
test_support_bundle.py::test_support_bundle_agent_with_node_selector PASSED
test_support_bundle.py::test_support_bundle_agent_with_taint_toleration[taint_nodes_exclude_self0] PASSED
test_support_bundle.py::test_support_bundle_agent_with_taint_toleration[taint_nodes_exclude_self1] PASSED

============================= 7 passed, 380 deselected in 507.19s (0:08:27) =============================

@c3y1huang c3y1huang marked this pull request as draft April 6, 2023 10:36
@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch 2 times, most recently from ea2744d to 247b621 Compare April 7, 2023 03:26
@c3y1huang
Copy link
Contributor Author

Test result: https://ci.longhorn.io/job/private/job/longhorn-tests-regression/3408/

@c3y1huang c3y1huang marked this pull request as ready for review April 7, 2023 03:27
This was referenced Apr 7, 2023
Merged
Merged
@c3y1huang
Copy link
Contributor Author

This should be merged with longhorn/longhorn-tests#1286.

Ref: longhorn/longhorn-tests#1286 (comment)

innobead
innobead previously approved these changes Apr 7, 2023
View reviewed changes
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few comments needed to resolve.

P.S. for common utils, epseically they could be shared with other modules, we can add to https://github.com/longhorn/go-common-lib.

controller/support_bundle_controller.go Outdated Show resolved Hide resolved
types/setting.go Outdated Show resolved Hide resolved
@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch 2 times, most recently from a0f838c to a4cb6f0 Compare April 7, 2023 07:23
@c3y1huang c3y1huang requested a review from innobead April 7, 2023 07:43
innobead
innobead requested changes Apr 7, 2023
View reviewed changes
controller/support_bundle_controller.go
@@ -615,6 +616,12 @@ func (c *SupportBundleController) createSupportBundleManagerDeployment(supportBu
func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhorn.SupportBundle,
nodeSelector map[string]string, tolerations []corev1.Toleration,
imagePullPolicy corev1.PullPolicy, priorityClass, registrySecret string) *appsv1.Deployment {

tolerationSetting, err := c.ds.GetSetting(types.SettingNameTaintToleration)
Copy link
Member

Choose a reason for hiding this comment

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

I mean you can use tolerations []corev1.Toleration instead of querying the setting again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I was confused because toleration []corev1.Toleration is unmarshalled from the setting, directly using it needs to be put back to a string again.

I've moved the unmarshalling into the same method so it won't be called again. PTAL, thank you.

@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch from a4cb6f0 to 4dc693e Compare April 7, 2023 08:57
@c3y1huang
fix(support-bundle): pass node-selector to agent
b8cc0fc 
Ref: 5614

Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
@c3y1huang
fix(support-bundle): pass taint-toleration to agent
7f4ac37 
Ref: 5614

Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
@c3y1huang
refactor(setting): unmarshal tolerations
c701184 
Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
@c3y1huang
test(types): UnmarshalTolerations
7c9ce22 
Signed-off-by: Chin-Ya Huang <chin-ya.huang@suse.com>
@c3y1huang c3y1huang force-pushed the fix-support-bundle-agent-missing-nodeselector-taintolerate branch from 4dc693e to 7c9ce22 Compare April 7, 2023 09:01
innobead
innobead approved these changes Apr 8, 2023
View reviewed changes
Copy link
Member

@innobead innobead left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. It's good that you added unit tests.

cc @longhorn/dev unit tests matter.

types/types_test.go
fmt.Printf("testing %v\n", name)

toleration, err := UnmarshalTolerations(test.input)
if !reflect.DeepEqual(toleration, test.expectedToleration) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: There should be some assert functions able to use?

types/types_test.go
t.Errorf("unexpected toleration:\nGot: %v\nWant: %v", toleration, test.expectedToleration)
}

if test.expectError && err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: The same. Suggest using assert like https://pkg.go.dev/github.com/stretchr/testify/assert.

@innobead
Copy link
Member

innobead commented Apr 8, 2023

@Mergifyio backport v1.4.x

@innobead innobead enabled auto-merge (rebase) April 8, 2023 01:15
@mergify
Copy link

mergify bot commented Apr 8, 2023 •
edited
Loading

backport v1.4.x

✅ Backports have been created

@innobead innobead disabled auto-merge April 8, 2023 01:27
@innobead innobead merged commit fee647c into longhorn:master Apr 8, 2023
@mergify mergify bot mentioned this pull request Apr 8, 2023
Merged
@c3y1huang c3y1huang deleted the fix-support-bundle-agent-missing-nodeselector-taintolerate branch April 10, 2023 01:34
@c3y1huang c3y1huang mentioned this pull request Apr 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@innobead innobead innobead approved these changes

Assignees

@c3y1huang c3y1huang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@c3y1huang @innobead