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.

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

✨ support add-on agent resource requirements settings #354

Conversation

elgnay
Copy link
Contributor

@elgnay elgnay commented Dec 28, 2024

Summary

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from mdelder and qiujian16 December 28, 2024 15:20
@elgnay
Copy link
Contributor Author

elgnay commented Dec 28, 2024

/hold

// +optional
// +listType=map
// +listMapKey=containerID
AgentResources []ContainerResourceRequirements `json:"agentResources,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

maybe just ResourceRequirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ResourceRequirements is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


// ContainerResourceRequirements defines resources required by one or a group of containers.
type ContainerResourceRequirements struct {
// ContainerID is a unique identifier for an agent container. It consists of three parts: resource kind,
Copy link
Member

Choose a reason for hiding this comment

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

is this kind or resource type? e.g. should user use Deployment or deployments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's resource kind, such as Deployment. Of course, it could be resource type as well. Which one is better?

Copy link
Member

Choose a reason for hiding this comment

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

it is better if it is group resource or group kind. Group might be needed since kind or resource names could be the same. Or alternatively, we only support certain type of resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the validation pattern to limit the supported resource types.

// multiple containers. For example, '*:*:*' matches all containers of the agent.
// +required
// +kubebuilder:validation:Required
// "+kubebuilder:validation:Pattern=`^.+:.+:.+$`
Copy link
Member

Choose a reason for hiding this comment

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

does this work with the begining of "?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation here is not strict. A value, like ":abc:def is allowed.

@elgnay elgnay force-pushed the addon-agent-res-setting branch from 0548fc4 to 3e993dc Compare January 6, 2025 09:42
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

Copy link
Contributor

openshift-ci bot commented Jan 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elgnay, qiujian16

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jan 6, 2025
@elgnay
Copy link
Contributor Author

elgnay commented Jan 9, 2025

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 1a5e25a into open-cluster-management-io:main Jan 9, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants