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

fix: set LHv2 DiskDriver to "auto" if unset #152

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Oct 10, 2024

Problem:
We need to force DiskDriver to "auto" if it's not explicitly set, because Longhorn also does that internally. If we don't do this, the subsequent reflect.DeepEqual() in LonghornV2Provisioner.Update() will always fail because we have an empty string, but the LHN CR will have it set to "auto" which results in a weird resync loop.

Solution:
This PR.

Related Issue:
harvester/harvester#6709

Test plan:
Add an LHv2 disk. Verify we don't see the errors mentioned in the related issue.

We need to force DiskDriver to "auto" if it's not explicitly set,
because Longhorn also does that internally.  If we don't do this,
the subsequent reflect.DeepEqual() in LonghornV2Provisioner.Update()
will always fail because we have an empty string, but the LHN CR
will have it set to "auto" which results in a weird resync loop.

Related issue: harvester/harvester#6709

Signed-off-by: Tim Serong <[email protected]>
@tserong
Copy link
Contributor Author

tserong commented Oct 11, 2024

harvester/vagrant-k3s#3 should fix that CI failure

Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Vicente-Cheng
Copy link
Collaborator

@Mergifyio backport v0.7.x

Copy link

mergify bot commented Oct 11, 2024

backport v0.7.x

✅ Backports have been created

@tserong
Copy link
Contributor Author

tserong commented Oct 11, 2024

latest CI failure is:

=== RUN   TestSingleDiskOperation/Test_1_UnprovisionSingleDisk
    test_0_single_disk_test.go:141: 
        	Error Trace:	/home/github/actions-runner/_work/node-disk-manager/node-disk-manager/tests/integration/test_0_single_disk_test.go:141
        	Error:      	Not equal: 
        	            	expected: "/var/lib/harvester/extra-disks/9665fd40086d48e7ca541b1e990c6645"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-/var/lib/harvester/extra-disks/9665fd40086d48e7ca541b1e990c6645
        	            	+
        	Test:       	TestSingleDiskOperation/Test_1_UnprovisionSingleDisk
        	Messages:   	Mountpoint should be empty after we remove disk!

I suspect this is probably because Test_1_UnprovisionSingleDisk() is still setting Spec.FileSystem.Provisioned = false rather than Spec.Provision = false, and is unrelated to this PR.

@Vicente-Cheng
Copy link
Collaborator

Yeah, and it should be fixed on #151. I thought we could merge it and let CI re-run with the master branch

@Vicente-Cheng Vicente-Cheng merged commit 09e26bc into harvester:master Oct 11, 2024
5 of 6 checks passed
@tserong tserong deleted the wip-fix-6709 branch October 11, 2024 09:55
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