Skip to content

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented Apr 19, 2025

What this PR does / why we need it

  • install lws controller in llmaz-system namespace

Which issue(s) this PR fixes

Fixes #202

Special notes for your reviewer

Does this PR introduce a user-facing change?

 migrate lws controller to llmaz-system namespace.

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Apr 19, 2025
@InftyAI-Agent InftyAI-Agent requested a review from kerthcet April 19, 2025 14:24
@googs1025
Copy link
Member Author

helm test:

root@VM-0-12-ubuntu:/home/ubuntu/llmaz# kubectl get pods -nllmaz-system
NAME                                        READY   STATUS    RESTARTS   AGE
llmaz-controller-manager-77978f9964-d8f5z   1/1     Running   0          7m46s
lws-controller-manager-7469fb5d9f-5z5fp     1/1     Running   0          7m46s
lws-controller-manager-7469fb5d9f-6bthh     1/1     Running   0          7m46s
prometheus-llmaz-prometheus-0               2/2     Running   0          7m46s
root@VM-0-12-ubuntu:/home/ubuntu/llmaz# kubectl get pods
NAME                                   READY   STATUS    RESTARTS   AGE
prometheus-operator-55b5c96cf8-q8vhd   1/1     Running   0          4d13h
qwen2-0--5b-0                          1/1     Running   0          5m4s


leaderWorkerSet:
enable: true
deployInSharedNamespace: false
Copy link
Member Author

Choose a reason for hiding this comment

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

This name doesn't seem to be appropriate, please let me know if there is a suitable one. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Can we just use a namespace field here?

Copy link
Member

Choose a reason for hiding this comment

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

If we want lws to install in lws-system, can we just remove the namespace field from helm charts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to make it clearer: if we use namespace: "llmaz-system" it will be deployed to llmaz-system, and if we set namespace: "test", then lws-controller will be deployed to the test namespace? In addition: if it is not set, it will be deployed in the lws-system namespace by default? 🤔

like this:

leaderWorkerSet:
  enable: true
  namesapce: "llmaz-system"

Copy link
Member

Choose a reason for hiding this comment

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

The simplest solution would be remove the namespace at all, then it will be installed in the same namespace with helm chart, usually the llmaz-system.

Another option is as you typed above, we have a namespace field here.

I'm ok with both approaches.

Copy link
Member

Choose a reason for hiding this comment

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

Let's pick the second then, to make it possible with current state -> lws in lws-system namespace.

Copy link
Member Author

Choose a reason for hiding this comment

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

for now:

set namespace: "llmaz-system" by default

leaderWorkerSet:
  enable: true
  namespace: "llmaz-system"

and so we can change to other namespace:

leaderWorkerSet:
  enable: true
  namespace: "test-system"

or no set, we will use lws-system

leaderWorkerSet:
  enable: true

@googs1025
Copy link
Member Author

Note: Currently I just use a bool to force installation to llmaz-system or lws-system. This may be a bit hardcoded. If you have other ideas, please let me know.

@googs1025
Copy link
Member Author

/kind feautre

@googs1025 googs1025 force-pushed the lws_namespace_to_llmaz_namespace branch from aaa96c7 to 9c03896 Compare April 19, 2025 14:32
@googs1025
Copy link
Member Author

/kind feature

@InftyAI-Agent InftyAI-Agent added feature Categorizes issue or PR as related to a new feature. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Apr 19, 2025
@kerthcet
Copy link
Member

kerthcet commented Apr 21, 2025

In the future, I would suggest to solve this in the upstream, introduce namespace in helm chart specifically. But good to us now.

@googs1025 googs1025 force-pushed the lws_namespace_to_llmaz_namespace branch 2 times, most recently from f6fcbb8 to d31b06d Compare April 22, 2025 01:48
@googs1025
Copy link
Member Author

test local:

root@VM-0-12-ubuntu:/home/ubuntu/llmaz# kubectl get pods -A
NAMESPACE            NAME                                             READY   STATUS    RESTARTS   AGE
default              prometheus-operator-55b5c96cf8-q8vhd             1/1     Running   0          7d
default              qwen2-0--5b-0                                    1/1     Running   0          116s
kube-system          coredns-668d6bf9bc-75wmc                         1/1     Running   0          7d
kube-system          coredns-668d6bf9bc-9twhj                         1/1     Running   0          7d
kube-system          etcd-cluster1-control-plane                      1/1     Running   0          7d
kube-system          kindnet-h2gpg                                    1/1     Running   0          7d
kube-system          kindnet-jznjw                                    1/1     Running   0          7d
kube-system          kindnet-sxj8d                                    1/1     Running   0          7d
kube-system          kube-apiserver-cluster1-control-plane            1/1     Running   0          7d
kube-system          kube-controller-manager-cluster1-control-plane   1/1     Running   0          7d
kube-system          kube-proxy-4pqlp                                 1/1     Running   0          7d
kube-system          kube-proxy-8ddvw                                 1/1     Running   0          7d
kube-system          kube-proxy-dv856                                 1/1     Running   0          7d
kube-system          kube-scheduler-cluster1-control-plane            1/1     Running   0          7d
llmaz-system         llmaz-controller-manager-77978f9964-ff7vt        1/1     Running   0          2m16s
llmaz-system         lws-controller-manager-7469fb5d9f-kfv5b          1/1     Running   0          2m16s
llmaz-system         lws-controller-manager-7469fb5d9f-r5mhw          1/1     Running   0          2m16s
llmaz-system         prometheus-llmaz-prometheus-0                    2/2     Running   0          2m15s
local-path-storage   local-path-provisioner-7dc846544d-pgrg7          1/1     Running   0          7d
root@VM-0-12-ubuntu:/home/ubuntu/llmaz# cat chart/values.global.yaml | grep namespace
  namespace: "llmaz-system"

@googs1025 googs1025 force-pushed the lws_namespace_to_llmaz_namespace branch from d31b06d to 42ebd91 Compare April 22, 2025 02:06
@kerthcet
Copy link
Member

kerthcet commented Apr 22, 2025

could you rebase and and add Action Required: to the user-facing change since we'll need to migrate the lws to another namespace.

@googs1025 googs1025 force-pushed the lws_namespace_to_llmaz_namespace branch from 42ebd91 to fc79203 Compare April 22, 2025 02:46
@googs1025 googs1025 force-pushed the lws_namespace_to_llmaz_namespace branch from fc79203 to 26703b9 Compare April 22, 2025 02:57
@googs1025
Copy link
Member Author

could you rebase and and add Action Required: to the user-facing change since we'll need to migrate the lws to another namespace.

done rebase and release note :)

@kerthcet
Copy link
Member

/lgtm
/approve

Thanks!

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 22, 2025
@InftyAI-Agent InftyAI-Agent merged commit 14174d6 into InftyAI:main Apr 22, 2025
19 checks passed
@carlory carlory mentioned this pull request May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. feature Categorizes issue or PR as related to a new feature. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install lws controller together with llmaz controller in the same namespace
3 participants