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
Add pod importer #1825
Add pod importer #1825
Conversation
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
f545a9f
to
00aa28a
Compare
Could you add README file with short description how to use the script, please? |
- The label key (`queuelabel`) providing the queue mapping is provided. | ||
- A mapping from ane of the encountered `queuelabel` values to an existing LocalQueue exists. | ||
- The LocalQueues involved in the import are using an existing ClusterQueue. | ||
- The ClusterQueues involved have at least one ResourceGroup using an existing ResourceFlavor. This ResourceFlavor is used when the importer creates the admission for the created workloads. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why at least one? What will happen if it uses non-existing ResourceFlavors?
Probably we should at least log warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the first resource flavor is not created, the check will fail.
cmd/importer/README.md
Outdated
### Example | ||
|
||
```bash | ||
./bin/importer import -n ns1,ns2 --queuelabel=src.lbl --queuemapping=src-val=user-queue,src-val=user-queue --dry-run=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why queuemapping has two identical parameters with src-val
label, which was not specified in --queuelabel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why queuemapping has two identical parameters with src-val label
fixed
cmd/importer/pod/check.go
Outdated
log := ctrl.LoggerFrom(ctx) | ||
log.Info("Check done", "checked", summary.TotalPods, "failed", summary.FailedPods) | ||
for e, pods := range summary.ErrorsForPods { | ||
log.Info("Error", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Info("Error", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) | |
log.Info("Check failed for Pod", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/importer/pod/import.go
Outdated
log := ctrl.LoggerFrom(ctx) | ||
log.Info("Import done", "checked", summary.TotalPods, "failed", summary.FailedPods) | ||
for e, pods := range summary.ErrorsForPods { | ||
log.Info("Error", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Info("Error", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) | |
log.Info("Could not import Pod", "err", e, "occurrences", len(pods), "obsevedFirstIn", pods[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, klog.KRef(pods[0])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pods is []string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/importer/pod/check.go
Outdated
return fmt.Errorf("%q flavor %q: %w", cq.Name, rf, util.ErrCQInvalid) | ||
} | ||
|
||
// do some additional checks like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's enhance the dry run:
Let's produce a list of the assignments to be done for each pod (local queue name, cluster queue name, flavor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As V(2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someting like "log.Info("Check succeeded" , "pod", kref.., "lq", LocalQueue ....)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe more like:
Dry run: Pod to be imported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
LGTM label has been added. Git tree hash: 45d53443f4403e435ec314b656d77f7537bf3f00
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, trasc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* [pod] Export `FromObject()` * [cmd/importer] Initial implementation. * Review Remarks * Add extra labels * Review Remarks * Review Remarks * Review Remarks * Review Remarks
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add a CLI tool able to import existing pods into Kueue.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?