Skip to content

Conversation

@ncr38
Copy link

@ncr38 ncr38 commented Sep 29, 2025

No description provided.

@openshift-ci openshift-ci bot requested review from deads2k and qiujian16 September 29, 2025 09:03
@openshift-ci
Copy link

openshift-ci bot commented Sep 29, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ncr38
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval. For more information see the 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

CascadeDeletionPolicy CascadeDeletionPolicy `json:"cascadeDeletionPolicy,omitempty"`

// New field for clusterSet-level overrides
ClusterSetOverrides []ClusterSetManifestOverride `json:"clusterSetOverrides,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I would name it as Override, and user can choose to override by clusterset, or placement or per cluster. Try to make it extensible enough.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. In that case , We'll need to have this logic to iterate from Coarse Grained selector to Fine Grained selector while creating manifest works as same resource can be selected by two selectors.
Placement Override -> ClusterSet Override -> Cluster Override

Copy link
Member

Choose a reason for hiding this comment

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

Maybe only separate by type, e.g.

overrides:
  overrideStrategy: clustersets
   clustersets:
   - name: clusterset1
     patch: .....
  - name: clusterset2

and we can also choose placements

overrides:
  overrideStrategy: placements
   clustersets:
   - name: aws-placement
     patch: .....
  - name: gke-placement


type ResourceIdentifier struct {
APIVersion string `json:"apiVersion,omitempty"`
Kind string `json:"kind,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

APIGroup/Version/Resource is preferred. We can reuse the existing resourceIdentifier

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will use the existing resource identifier.


type OverrideSpec struct {
// Metadata overrides (name, namespace, labels, annotations)
MetadataOverride *MetadataOverride `json:"metadataOverride,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

What does this for, why it cannot be part of general override?

Copy link
Author

Choose a reason for hiding this comment

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

Initially thought of allowing template based override in metadataOverride like labels.cluster : {{clusterName}}

Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need to treat spec and metadata differently.

MetadataOverride *MetadataOverride `json:"metadataOverride,omitempty"`

// JSON patch operations for spec modifications
SpecOverride []JSONPatch `json:"specOverride,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I would make it more extensible, someone might want to use json merge patch also.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, I will introduce Replace , StrategicMerge as two possible strategies for patching things.

Copy link
Member

Choose a reason for hiding this comment

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

StrategicMerge will be tricky...we can start with JSONPatch/JSONMerge/Replace

value: 500m
- op: replace
path: /spec/template/spec/containers/0/resources/requests/memory
value: 512Mi
Copy link
Member

Choose a reason for hiding this comment

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

what the type of the value it should be? number or string?

metadataOverride:
labels:
environment: production
tier: high-performance
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 part a merge or replace operation?

Copy link
Author

Choose a reason for hiding this comment

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

replace

- **Multi-environment deployments**: Deploy the same application with environment-specific configurations (prod clusterSet: 5 replicas, staging clusterSet: 1 replica)
- **ClusterSet capacity optimization**: Adjust resource requests/limits based on clusterSet capacity profiles
- **Compliance requirements**: Apply different security contexts or labels based on clusterSet compliance levels
- **Regional customizations**: Use different images or configurations for different geographical clusterSets
Copy link
Member

Choose a reason for hiding this comment

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

Another use case would be different configurations in different cloud providers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants