Skip to content

[API] introduce the logic of artifact deletion considering artifact#502

Merged
slin1237 merged 1 commit into
mainfrom
private-artifact-reconcile
Jan 13, 2026
Merged

[API] introduce the logic of artifact deletion considering artifact#502
slin1237 merged 1 commit into
mainfrom
private-artifact-reconcile

Conversation

@truddy0
Copy link
Copy Markdown
Collaborator

@truddy0 truddy0 commented Jan 12, 2026

What this PR does

This is the 2nd PR of model artifact reuse feature #409 .

Introduce the logic of artifact deletion considering artifact attribute in configmap
Deletion logic:

  1. if the artifact directory does not have any symbolic link pointed to, remove the artifact directory. Otherwise, skip deleting artifact directory
  2. if the parent artifact directory is dangling after the last child is deleted, delete the parent artifact directory after the last child artifact is deleted
    Change internal contract:
  3. the arfiact attribute on configmap is
type Artifact struct {
	Sha string `json:"sha"` // sha string fetched from HuggingFace
	// parent model name -> parent model artifact storage path
	ParentPath    map[string]string `json:"parentPath"`
	ChildrenPaths []string          `json:"childrenPaths"` // an array of children paths
}

Why we need it

efficiently remove the artifact directory without interfering with the usage

How to test

Test ID Context Test Action Expected Result Test Evidence
Test 1 Parent model has 1 child Delete parent model Parent model artifact is preserved Test 1 in test evidence file
Test 2 Parent model has multiple children Delete a child model while parent still has other children Child artifact directory is deleted; parent artifact directory is preserved; parent CM entry is updated (kid1 removed, kid2 remains) Test 2 in test evidence file
Test 3 Parent model CR is already deleted; parent still has more than 1 children Delete child model Child artifact directory is deleted; parent artifact directory is preserved Test 3 in test evidence file
Test 4 Parent model CR is already deleted; this is the last remaining child Delete child model Child artifact directory is deleted; parent artifact directory is deleted as well Test 4 in test evidence file

testEvidence-ModelDeletion.txt

Checklist

  • Tests added/updated (if applicable)
  • Docs updated (if applicable)
  • make test passes locally

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added model-agent Model agent changes tests Test changes labels Jan 12, 2026
@slin1237
Copy link
Copy Markdown
Collaborator

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces significant logic for artifact deletion and reuse, including a parent-child relationship for artifacts stored in the ConfigMap. The changes are extensive, touching config parsing, ConfigMap reconciliation, and the main gopher logic. The code is well-refactored into smaller, more manageable functions, especially the new deletion logic in gopher.go and the robust ConfigMap update mechanisms in configmap_reconciler.go. The accompanying tests are thorough and cover many new scenarios and edge cases. I have a couple of minor suggestions for improvement.

Comment thread pkg/modelagent/configmap_reconciler.go Outdated
// Store the model entry in the ConfigMap
configMap.Data[key] = string(entryJSON)

c.logger.Infof("will update model profress in configmap for key %s", key)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There's a typo in the log message. "profress" should be "progress".

Suggested change
c.logger.Infof("will update model profress in configmap for key %s", key)
c.logger.Infof("will update model progress in configmap for key %s", key)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

fixed the typo

Comment thread pkg/modelagent/model_data.go Outdated
Sha: currentArtifact.Sha,
ParentPath: currentArtifact.ParentPath,
ChildrenPaths: append([]string(nil), currentArtifact.ChildrenPaths...),
ChildrenPaths: currentArtifact.ChildrenPaths,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The ChildrenPaths slice is now assigned directly, whereas previously a copy was made using append. While this might be safe in the current context as the resulting ModelConfig is short-lived, it's generally safer to create a copy to prevent potential side effects if the source or destination slice is modified elsewhere. This makes the code more robust against future changes.

Suggested change
ChildrenPaths: currentArtifact.ChildrenPaths,
ChildrenPaths: append([]string(nil), currentArtifact.ChildrenPaths...),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

introduce logics to handle edge case for both parentPath and chidlrenPaths

…ttribute in configmap

1. if the artifact directory does not have any symbolic link pointed to, remove the artifact directory. Otherwise, skip deleting artifact directory
2. if the parent artifact directory is dangling after the last child is deleted, delete the parent artifact directory after the last child artifact is deleted
@truddy0 truddy0 force-pushed the private-artifact-reconcile branch from 55cb5fb to 472ed8d Compare January 13, 2026 18:04
@slin1237 slin1237 merged commit 8edd7ac into main Jan 13, 2026
29 checks passed
@slin1237 slin1237 deleted the private-artifact-reconcile branch January 13, 2026 20:38
pallasathena92 pushed a commit that referenced this pull request Jan 20, 2026
…502)

[API] introduce the logic of artifact deletion considering artifact attribute in configmap
1. if the artifact directory does not have any symbolic link pointed to, remove the artifact directory. Otherwise, skip deleting artifact directory
2. if the parent artifact directory is dangling after the last child is deleted, delete the parent artifact directory after the last child artifact is deleted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

model-agent Model agent changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants