Skip to content

Adding check on cd status #969

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

Closed
visheshtanksale wants to merge 1 commit into from
+19 −0
Closed

Adding check on cd status #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions cmd/compute-domain-controller/cdstatus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,22 @@ func (m *ComputeDomainStatusManager) getNonStaleFabricNodes(existingNodes []*nva
return result
}

func validateCDStatusNodes(cd *nvapi.ComputeDomain) {
type cliqueIndexKey struct {
cliqueID string
index int
}
seen := make(map[cliqueIndexKey]string)
for _, node := range cd.Status.Nodes {
key := cliqueIndexKey{cliqueID: node.CliqueID, index: node.Index}
if prevNodeName, exists := seen[key]; exists {
klog.Fatalf("ComputeDomain %s/%s status invariant violated: node index %d in clique %q is used by both node %q and node %q",
cd.Namespace, cd.Name, node.Index, node.CliqueID, prevNodeName, node.Name)
}
seen[key] = node.Name
}
}

// nodesEqual checks if two slices of ComputeDomainNode are equal.
func (m *ComputeDomainStatusManager) nodesEqual(a, b []*nvapi.ComputeDomainNode) bool {
aMap := make(map[string]nvapi.ComputeDomainNode)
Expand Down
3 changes: 3 additions & 0 deletions cmd/compute-domain-controller/computedomain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,9 @@ func (m *ComputeDomainManager) UpdateStatus(ctx context.Context, cd *nvapi.Compu
// Recalculate global status based on current state
cd.Status.Status = m.calculateGlobalStatus(cd)

// Validate status invariants: crash loudly if a node index is used more than once.
validateCDStatusNodes(cd)
Copy link
Copy Markdown
Contributor

@jgehrcke jgehrcke Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should do this in the controller.

The original ticket that I believe you are taking reference to was more about an assert-style invariant check in the direct write/update code path.

This may actually be implied as of today, the code evolved a bit.

Copy link
Copy Markdown
Contributor Author

@visheshtanksale visheshtanksale Mar 31, 2026

Choose a reason for hiding this comment

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

@jgehrcke I think this is the code that ensures the index is not duplicate. We dont need this implementation now. Closing this PR


updatedCD, err := m.config.clientsets.Nvidia.ResourceV1beta1().ComputeDomains(cd.Namespace).UpdateStatus(ctx, cd, metav1.UpdateOptions{})
if err != nil {
return nil, err
Expand Down
Loading