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

test/includes/microcloud: Remove core.trust_password support #323

Merged
merged 6 commits into from
Jun 21, 2024
Merged

test/includes/microcloud: Remove core.trust_password support #323

merged 6 commits into from
Jun 21, 2024

Conversation

roosterfish
Copy link
Contributor

This is required after canonical/lxd#13567. Instead of the password we now have to use a token when adding the remote.

@roosterfish roosterfish changed the title test/includes/microcloud: Remove core.trust_password support test/includes/microcloud: Remove core.trust_password support Jun 17, 2024
@roosterfish roosterfish marked this pull request as draft June 17, 2024 13:25
@roosterfish
gomod: Update dependencies
4e60a31 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish
github: Increase min Go version to 1.22.3
ae68b6d 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish roosterfish force-pushed the replace_trust_pw branch 2 times, most recently from 7f6681b to 4ce75af Compare June 17, 2024 13:38
simondeziel
simondeziel previously approved these changes Jun 17, 2024
View reviewed changes
@roosterfish roosterfish marked this pull request as ready for review June 17, 2024 14:30
@roosterfish
service: Use token field in configFromToken
6043e39 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish
test/includes/microcloud: Remove core.trust_password support
dd2b538 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
simondeziel
simondeziel previously approved these changes Jun 20, 2024
View reviewed changes
@roosterfish
service: Fix error message
79a7655 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish
Copy link
Contributor Author

Will mark as draft until I have finalized the testing on my end.

@roosterfish roosterfish marked this pull request as draft June 20, 2024 15:41
@roosterfish
service: Run GetServer before checking API extensions
393b42a 
Signed-off-by: Julian Pelizäus <julian.pelizaeus@canonical.com>
@roosterfish roosterfish marked this pull request as ready for review June 21, 2024 09:35
@roosterfish
Copy link
Contributor Author

That is now ready for a final review. I have tested forming LXD clusters in version 5.21 (prior to password removal) and edge.

There was an error in the HasExtension check as it will always return true if the underlying LXD client has never before done a call to GetServer() which will fetch and cache the API extensions. See https://github.com/canonical/lxd/blob/main/client/lxd_server.go#L62.
A fix for this is added to this PR.

@roosterfish
Copy link
Contributor Author

The pipeline is now failing due to a regression caused by canonical/lxd-pkg-snap#471.

masnax
masnax reviewed Jun 21, 2024
View reviewed changes
service/lxd.go
@@ -309,6 +309,14 @@ func (s *LXDService) HasExtension(ctx context.Context, target string, address st
}
}

// Fill the cache of API extensions.
// If the client's internal `server` field isn't yet populated
// a call to HasExtension will always return true for any extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a corresponding fix for this in LXD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed earlier I'll follow up on this in LXD.

masnax
masnax approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@masnax masnax 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 @roosterfish

simondeziel
simondeziel approved these changes Jun 21, 2024
View reviewed changes
service/lxd.go
@@ -200,7 +200,7 @@ func (s LXDService) Join(ctx context.Context, joinConfig JoinConfig) error {

err = op.WaitContext(ctx)
if err != nil {
return fmt.Errorf("Failed to configure cluster :%w", err)
return fmt.Errorf("Failed to configure cluster: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

It seems to have turn up a No matching cluster join operation found error in https://github.com/canonical/microcloud/actions/runs/9611369723/job/26518376077?pr=323#step:13:58498

That's an error I also saw pop-up from time to time in the LXD test suite. Anyway, not relevant for this PR but would be nice to have that one addressed in LXD ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it might be a race between creating the operation and checking for it.

@simondeziel
Copy link
Member

The pipeline is now failing due to a regression caused by canonical/lxd-pkg-snap#471.

This was fixed by canonical/lxd-pkg-snap#476 and now tests are working.

@masnax masnax merged commit 52839a2 into canonical:main Jun 21, 2024
14 of 15 checks passed
@roosterfish roosterfish deleted the replace_trust_pw branch June 21, 2024 16:13
@roosterfish roosterfish mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masnax masnax masnax approved these changes

@simondeziel simondeziel simondeziel approved these changes

@tomponline tomponline Awaiting requested review from tomponline

@markylaing markylaing Awaiting requested review from markylaing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@roosterfish @simondeziel @masnax