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

[WIP] GPU/TPU resource-collector add-on #16

Closed
wants to merge 13 commits into from

Conversation

z1ens
Copy link
Contributor

@z1ens z1ens commented Jul 2, 2024

Copy link

openshift-ci bot commented Jul 2, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: z1ens
Once this PR has been reviewed and has the lgtm label, please assign mikeshng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

agentSpec:
workload:
manifests:
- kind: ClusterRole
Copy link
Member

Choose a reason for hiding this comment

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

Could you try to minimize the rbac permission?

clusterSet: global

---
apiVersion: cluster.open-cluster-management.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

suggest to move placement to a seperate yaml.

@haoqing0110
Copy link
Member

haoqing0110 commented Jul 8, 2024

IMAGE ?= resource-usage-collect-addon
IMAGE_REGISTRY ?= quay.io/haoqing
IMAGE_TAG ?= latest
IMAGE ?= resource-usage-collect-addon-template
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 not necessary to add the -template suffix.

apiVersion: cluster.open-cluster-management.io/v1beta2
kind: ManagedClusterSetBinding
metadata:
generation: 1
Copy link
Member

Choose a reason for hiding this comment

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

the generation line is not needed.

type: Placements
placements:
- name: placement-all
namespace: default
Copy link
Member

Choose a reason for hiding this comment

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

EOF, can we add a blank line for all yaml files?

@zhujian7
Copy link
Member

zhujian7 commented Jul 8, 2024

it seems the DCO check is not working now. I will try to fix it.

@zhujian7 zhujian7 mentioned this pull request Jul 8, 2024
@zhujian7
Copy link
Member

zhujian7 commented Jul 8, 2024

it seems the DCO check is not working now. I will try to fix it.

A DCO check has been added #17, @z1ens could you rebase and sign off your commits(git commit --signoff)?

@z1ens z1ens changed the title [WIP] adjust to template [WIP] GPU/TPU resource-collector add-on Jul 8, 2024
@@ -12,4 +12,5 @@
*.out

# Dependency directories (remove the comment below to include it)
# vendor/
# vendor/.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

should .DS_Store be in a new line? It is not a sub folder of the vendor, right?

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.

3 participants