Skip to content

Conversation

@AmitSahastra
Copy link
Contributor

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements hostname-based naming conventions for LXD host registration with MAAS instead of using IP addresses, along with significant code refactoring to improve maintainability and structure.

  • Use hostname-based naming convention (lxd-host-<hostname>) for LXD host registration instead of IP-based naming
  • Refactor machine deletion logic into smaller, more focused functions
  • Update LXD initializer to use normalized node names and clean up commented code

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/maas/lxd/host_maas_client.go Enhanced registration logic to use hostname-based naming with fallback to IP, refactored function signatures
lxd-initializer/lxd-initializer.go Updated to use normalized node names for host registration and removed commented code
lxd-initializer/lxd-initializer-daemonset.yaml Updated container image version
controllers/templates/lxd_initializer_ds.yaml Updated container image version
controllers/maasmachine_controller.go Major refactoring of deletion logic into smaller functions and hostname-based LXD host operations
controllers/lxd_initializer_ds.go Refactored large function into smaller, focused helper functions for better maintainability

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +248 to +251
hostName := canonicalLXDHostName(m.Hostname)
if hostName == "lxd-host-" {
return
}
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

The check for empty hostname suffix is fragile. If m.Hostname is empty or whitespace-only, canonicalLXDHostName will return 'lxd-host-' which matches this condition, but this logic assumes the hostname is always non-empty after trimming. Consider checking strings.TrimSpace(m.Hostname) == \"\" directly before calling canonicalLXDHostName.

Copilot uses AI. Check for mistakes.
Comment on lines +278 to +281
hostName := canonicalLXDHostName(m.Hostname)
if hostName == "lxd-host-" {
return err
}
Copy link

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

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

Same issue as above - the check for empty hostname suffix is duplicated and fragile. Consider extracting this validation into a helper function or checking the hostname before calling canonicalLXDHostName.

Copilot uses AI. Check for mistakes.
@AmitSahastra AmitSahastra changed the title Use hsotname for lxd registration. Code refactoring. Use hostname for lxd registration. Code refactoring. Sep 17, 2025
@AmitSahastra AmitSahastra merged commit 462ed8c into PCP-5152 Sep 17, 2025
4 checks passed
@AmitSahastra AmitSahastra deleted the PCP-5152_2 branch September 17, 2025 09:43
AmitSahastra added a commit that referenced this pull request Sep 17, 2025
* PCP-5152: Multiple node HCP cluster losing interface reference

* Additional changes (#225)

* Additional changes

* trust password and code cleanup (#226)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup. (#234)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* - Gosec fix
- Cleanup daemonset once all host are registered

* Additional changes

* Use hsotname for lxd registration. Code refactoring. (#240)

* Use hsotname for lxd registration. Code refactoring.

* merge conflict
github-actions bot pushed a commit that referenced this pull request Sep 17, 2025
* PCP-5152: Multiple node HCP cluster losing interface reference

* Additional changes (#225)

* Additional changes

* trust password and code cleanup (#226)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup. (#234)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* - Gosec fix
- Cleanup daemonset once all host are registered

* Additional changes

* Use hsotname for lxd registration. Code refactoring. (#240)

* Use hsotname for lxd registration. Code refactoring.

* merge conflict

(cherry picked from commit 5939162)
AmitSahastra added a commit that referenced this pull request Sep 17, 2025
…#244)

* PCP-5152: Multiple node HCP cluster losing interface reference

* Additional changes (#225)

* Additional changes

* trust password and code cleanup (#226)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup. (#234)

* Additional changes

* Add gate to registration logic, seperate it to initializer and put grace period. Code cleanup.

* - Gosec fix
- Cleanup daemonset once all host are registered

* Additional changes

* Use hsotname for lxd registration. Code refactoring. (#240)

* Use hsotname for lxd registration. Code refactoring.

* merge conflict

(cherry picked from commit 5939162)

Co-authored-by: Amit Sahastrabuddhe <[email protected]>
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.

2 participants