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

341 enable defining additional custom taints and labels to virtual nodes from interlink config #362

Conversation

Bianco95
Copy link
Collaborator

Refactor of the Configuration Structure for VK

This PR introduces improvements and refactoring to the VK configuration structure to enhance clarity, usability, and flexibility.

Key Changes:

  1. Refactor of the Configuration Keys:

    • Configuration keys no longer start with capital letters, ensuring consistency with YAML conventions.
    • Hierarchical structure improved for better organization and readability, particularly within the resources section.
  2. Dynamic Handling of Resources:

    • The new resources structure supports dynamic configuration of accelerators such as GPUs and FPGAs.
    • Example configuration:
      virtualNode:
        resources:
          cpus: 8
          memgib: 49
          pods: 100
          accelerators:
          - resource_type: nvidia.com/gpu
            model: t4
            available: 1
          - resource_type: nvidia.com/gpu
            model: A100
            available: 2
          - resource_type: xilinx.com/fpga
            model: U55C
            available: 1
        nodeLabels:
          - "virtual-kubelet=interlink"
          - "cool-node=true"
        nodeTaints:
          - key: "accelerator"
            value: "A100"
            effect: "NoSchedule"
  3. Support for Custom Node Labels and Taints:

    • Additional node labels and taints can now be specified in the configuration.
    • Taints and labels provide flexibility for resource allocation and workload scheduling.
  4. Automatic Node Labeling for Accelerators:

    • The VK automatically adds labels for accelerators based on the configuration.
    • Example: If an NVIDIA GPU model A100 is specified, the label nvidia-gpu-type=a100 is added to the node.
  5. Helm Chart must be updated accordingly:

    • The Helm chart should be updated accordingly to support the new configuration structure.
    • Example Helm configuration:
      apiVersion: v1
      kind: ConfigMap
      metadata:
        name: "{{ .Values.nodeName }}-virtual-kubelet-config"
        namespace: {{ .Release.Namespace }}
      data:
        InterLinkConfig.yaml: |
          {{- if .Values.interlink.socket }}
          interlinkURL: {{ .Values.interlink.socket | quote }}
          InterlinkPort: {{ printf "%s-1" .Values.interlink.socket | quote }}
          {{- else }}
          interlinkURL: {{ .Values.interlink.address | quote }}
          interlinkPort: {{ .Values.interlink.port | quote }}
          {{- end }}
          exportPodData: {{ .Values.interlink.exportPodData }}
          verboseLogging: true
          errorsOnlyLogging: false
          serviceAccount: "{{ .Values.nodeName }}"
          namespace: "{{ .Release.Namespace }}"
          vkTokenFile: {{ .Values.OAUTH.enabled | ternary "/opt/interlink/token" "/dev/null" }}
          resources:
            cpu: "{{ .Values.virtualNode.resources.cpus }}"
            memory: "{{ .Values.virtualNode.resources.memgib }}Gi"
            pods: "{{ .Values.virtualNode.resources.pods }}"
            accelerators:
            {{- range .Values.virtualNode.resources.accelerators }}
            - resource_type: "{{ .resource_type }}"
              model: "{{ .model }}"
              available: {{ .available }}
            {{- end }}
          http:
            insecure: {{ .Values.virtualNode.http.insecure }}
          kubeletHTTP:
            insecure: {{ .Values.virtualNode.kubeletHTTP.insecure }}
          nodeLabels:
            {{- range .Values.virtualNode.nodeLabels }}
            - "{{ . }}"
            {{- end }}
          nodeTaints:
            {{- range .Values.virtualNode.nodeTaints }}
            - key: "{{ .key }}"
              value: "{{ .value }}"
              effect: "{{ .effect }}"
            {{- end }}

@dciangot
Copy link
Collaborator

@Bianco95 I think the name on the configuration should be fixed on the helm chart as well before getting this done. https://github.com/interTwin-eu/interlink-helm-chart/

can you check it please?

@dciangot
Copy link
Collaborator

on a second thought (I can feel your hate already @Bianco95 ). I do think it would be better to include this functionality without touching the current config capitalization.

So this PR can go while we think at the best naming for the config entries.

@Bianco95
Copy link
Collaborator Author

We need to maintain the capitalization as shown (for example, the Config struct for the VK should be written as follows):

package virtualkubelet

// Config holds the entire configuration
type Config struct {
	InterlinkURL      string      `yaml:"InterlinkURL"`
	InterlinkPort     string      `yaml:"InterlinkPort"`
	VKConfigPath      string      `yaml:"VKConfigPath"`
	VKTokenFile       string      `yaml:"VKTokenFile"`
	ServiceAccount    string      `yaml:"ServiceAccount"`
	Namespace         string      `yaml:"Namespace"`
	PodIP             string      `yaml:"PodIP"`
	VerboseLogging    bool        `yaml:"VerboseLogging"`
	ErrorsOnlyLogging bool        `yaml:"ErrorsOnlyLogging"`
	HTTP              HTTP        `yaml:"HTTP"`
	KubeletHTTP       HTTP        `yaml:"KubeletHTTP"`
	Resources         Resources   `yaml:"Resources"`
	NodeLabels        []string    `yaml:"NodeLabels"`
	NodeTaints        []TaintSpec `yaml:"NodeTaints"`
}

This format should be preserved, while also keeping the new features introduced intact, correct?

@dciangot
Copy link
Collaborator

dciangot commented Feb 3, 2025

@Bianco95 these tests are failing, I suppose you still working on this, right?

@Bianco95
Copy link
Collaborator Author

Bianco95 commented Feb 3, 2025

yes, I've been fighting with capital letters, but I think I won finally

Copy link
Collaborator

@dciangot dciangot left a comment

Choose a reason for hiding this comment

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

LGTM

@dciangot
Copy link
Collaborator

dciangot commented Feb 3, 2025

@Bianco95 also the installer should be fixed accordingly to support at least the core resources: https://github.com/interTwin-eu/interLink/blob/main/cmd/installer/templates/values.yaml

@dciangot
Copy link
Collaborator

dciangot commented Feb 3, 2025

also there is a conflict for the newly introduced kubernetes service account endpoints configurations

…ble-defining-additional-custom-taints-and-labels-to-virtual-nodes-from-interlink-config
…ble-defining-additional-custom-taints-and-labels-to-virtual-nodes-from-interlink-config
@dciangot dciangot merged commit 034964c into main Feb 5, 2025
6 checks passed
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.

Enable defining additional custom taints and labels to virtual nodes from interlink config
2 participants