Fix issue #26: Cleanup and simplify code logic #28

jiere · 2023-07-08T13:07:17Z

See #26

Major change:

For kubectl and kind, using Github-hosted runners' provisioned version.
Simplify code logic, remove some unnecessary ENV options.
Update README.md and GHA workflow test.yml accordingly.

After this PR be merged, will cleanup related code in kepler-action repo accordingly.

Signed-off-by: Jie Ren [email protected]

jiere · 2023-07-08T13:09:50Z

@SamYuan1990 , Please help review, thanks.

SamYuan1990 · 2023-07-09T03:11:42Z

I am not sure. @marceloamaral , if we are considering with local dev env step up usage.... we may have to block this PR or at least a change request here.
@jiere , if further considering with self host runner, after this PR, the self host runner have to maintainer their own kubectl and kind version?

jiere · 2023-07-09T08:47:46Z

if further considering with self host runner, after this PR, the self host runner have to maintainer their own kubectl and kind version?

Agree, prerequisite well-known or popular packages should be maintained by machine owner, only those project specific tools need be taken care of by project's GHA code, IMO.

rootfs · 2023-07-09T13:40:20Z

cc @vprashar2929

SamYuan1990 · 2023-07-09T13:56:31Z

I suppose we need a discussion as do we need handle kubectl and kind install script or not. I am now open for it.

As one of the question covered in this PR.
if we apply with current version, which will not work for Mac as sed issue when local usage. (if we take a look at commit history?)

SamYuan1990 · 2023-07-09T14:03:06Z

common.sh

@@ -20,9 +20,6 @@
 set -ex
 set -o pipefail

-_registry_port="5001"


en.... I don't like force port number. I hope to keep it flexible.

The reason why I chose to fix the port here is that if remain such flexibility, then we have a very ugly sed scripts to replace the port info inside the local-registry.yml.
As you could see in current PR diff, I also cleaned up there to make code cleaner:)
Flexibility often comes with complexity:) So we may do trade-off.
Some of the options are not so important to users, such as the registry port, for local-dev use and CI use, IMO, fix to 5001 is acceptable.

if we have to flexible with port number, any better solution than sed?

Maybe we can use ex command like this:

ex -s -c "1,\$s/$kind_registry_name/${KIND_REGISTRY_NAME}/ge" -c "x" "${KIND_DIR}"/kind.yml

I think it is available on Mac and Linux

Maybe we can use ex command like this:

ex -s -c "1,\$s/$kind_registry_name/${KIND_REGISTRY_NAME}/ge" -c "x" "${KIND_DIR}"/kind.yml

I think it is available on Mac and Linux

In my dev machine, the ex points to the same binary as vim :-)
$ ls -l /usr/bin/ex
lrwxrwxrwx 1 root root 20 Aug 24 2021 /usr/bin/ex -> /etc/alternatives/ex
$ ls -l /usr/bin/vim
lrwxrwxrwx 1 root root 21 Aug 24 2021 /usr/bin/vim -> /etc/alternatives/vim
$ ls -l /etc/alternatives/ex
lrwxrwxrwx 1 root root 18 Aug 24 2021 /etc/alternatives/ex -> /usr/bin/vim.basic
$ ls -l /etc/alternatives/vim
lrwxrwxrwx 1 root root 18 Aug 24 2021 /etc/alternatives/vim -> /usr/bin/vim.basic

Yes, it's the line editor for vim 😅
It was new to me as well

em... well technologically maybe sed still one of best options?
I mean just when we replace string via shell.

This issue is fixed in #31 by using sed < manifest/yaml > generated/yaml

SamYuan1990 · 2023-07-09T14:05:01Z

kind/kind.sh

-    echo "Building manifests..."
-
-    cp $KIND_MANIFESTS_DIR/kind.yml "${KIND_DIR}"/kind.yml
-    sed "s/$kind_registry_name/${KIND_REGISTRY_NAME}/g" "${KIND_DIR}"/kind.yml > "${KIND_DIR}"/kind.yml.tmp && mv "${KIND_DIR}"/kind.yml.tmp "${KIND_DIR}"/kind.yml


sed has different behaviors on mac and linux. :-)

as I don't want to block mac user use this repo to setup their own dev cluster at local.
but... it seems the changes here is about do we need flexibility on port number?

Exactly, why not we fix the content of both kind.yml and local-registry.yml :)

vprashar2929 · 2023-07-10T04:59:06Z

I suppose we need a discussion as do we need handle kubectl and kind install script or not. I am now open for it.

I agree that we need a consensus on this. Do we want to use local-dev-cluster only for CI env setup or for local env setup as well?
IMO it is best that we have a way to handle these dependency installations inside the script as we refer to local-dev-cluster in many of our other repos(kepler,kepler-operator, etc) as part of the local dev environment setup plus it avoids any manual interventions to deal with dependencies whenever we need a quick setup to test kepler.
I think we can modify the script to make sure that if it is referred to in GHA then it should not care about handling kubectl and kind installation. @SamYuan1990 @jiere Wdyt?

jiere · 2023-07-10T06:08:34Z

Do we want to use local-dev-cluster only for CI env setup or for local env setup as well?

My answer is both. But the point here is that prerequisite package setup should be one-shot job, it should be handled by runner admin or dev cluster owner. IMO, such kind/kubectl installation job is similar as Golang setup.

IMO it is best that we have a way to handle these dependency installations inside the script as we refer to local-dev-cluster in many of our other repos kepler,kepler-operator, etc) as part of the local dev environment setup plus it avoids any manual interventions to deal with dependencies whenever we need a quick setup to test kepler.

I agree that we should give convenience for developers to quickly setup local dev cluster and do quick try on Kepler.
The reason I chose to make change here is that it is no so good to fix version of kind/kubectl, or even Golang here, since they are hot community and have quick version evolving, fix version means high risk on interoperation issues.
IMO the best practice is to simply let developers to keep them up-to-date, unless our project has specific version limitation dependency on specific tools.

Many famous and successful open source projects have their fundamental prerequisite package lists, install them either manually or through user local scripts does not mean manual invention since this is dev env setup, not the project itself stuffs.
The items mentioned in our document Prerequisite section means that they should be kept up-to-date or follow the version definition there.

SamYuan1990 · 2023-07-10T12:42:51Z

I suppose we need a discussion as do we need handle kubectl and kind install script or not. I am now open for it.

I agree that we need a consensus on this. Do we want to use local-dev-cluster only for CI env setup or for local env setup as well? IMO it is best that we have a way to handle these dependency installations inside the script as we refer to local-dev-cluster in many of our other repos(kepler,kepler-operator, etc) as part of the local dev environment setup plus it avoids any manual interventions to deal with dependencies whenever we need a quick setup to test kepler. I think we can modify the script to make sure that if it is referred to in GHA then it should not care about handling kubectl and kind installation. @SamYuan1990 @jiere Wdyt?

Do we want to use local-dev-cluster only for CI env setup or for local env setup as well?

Both.

I think we can modify the script to make sure that if it is referred to in GHA then it should not care about handling kubectl and kind installation.

@marceloamaral what do you think if we turn this repo from an empty linux to an environment same with today's GHA dependence?
as historical speaking, @marceloamaral is the most sponsor for local dev cluster at very beginning.

IMO,

It supports GHA, as current our pipeline we have.
It supports Mac OS, as I am Mac user.
as currently it may able to start from a prune linux OS, which means a self hosted instance can free from kubectl and kind version. @jiere it's looks wired at my point of view, as you request flexible for multiple OS support, but on the other hand... this PR is opened. ref to Kepler Action should support runners other than Ubuntu system. kepler-action#50 (comment), as we want to cover more CI tools, and if lucky, personally I hope to reuse currently code, hence I hope to keep robust, and avoid this repo force binding with GHA.

With a possible case, just assume this repo running on a self hosted instance, maybe triggered by GHA or not.
if we hard code as port number as 5001 which means for one time, we only have a single job can run. otherwise port conflicts. Hence further we need more flexibility and robust but not hard code.
May be further the cluster named kind is also a limitation, maybe the port and cluster name can be refined with a "random" value as $job_number+kind for port and $job_number as port.
And currently sed is a common way to replace the value. :-) if there any other good suggestion, please let me know.

SamYuan1990 · 2023-07-10T12:47:45Z

As for today, I keep my standard to review this PR as the same for https://github.com/sustainable-computing-io/local-dev-cluster/pull/9/files if it doesn't break CI, then I can accept with merge it.
But for further consideration, I suppose this PR or parts of this PR reduced our robust and flexibility. And in further if we considering with sustainable-computing-io/kepler-action#50 (comment) we have to add them back?(maybe not code back but feature back)

jiere · 2023-07-11T02:26:07Z

Thanks @SamYuan1990 's comments above, let me clarify two things:

This PR has no conflict with my earlier raised issue, in the mid-term or long-term, we should support more OS types of runners other than current Ubuntu. Current PR is discussing another topic, for any runner OS, shall we local-dev-cluster repo code take care of those prerequisite package installation stuffs? In my point of view, no matter which OS we are running on the runner, such package installation stuffs should be taken care of by the runner admin, but not our code. Take Github-hosted runner as example, Microsoft/Github guys are working hard to maintain their runners, even create repo for it, and also setup related policies. Today, we can use functions such as action/setup-go in our GHA workflow yml just because they have written code for it, if no such code maintained by the runner admin, do we need to add the golong installation code in our local-dev-cluster repo also?

if we hard code as port number as 5001 which means for one time, we only have a single job can run. otherwise port conflicts.

You persuaded me successfully for above reason, also thanks @vprashar2929 for your suggestions above, let me try if that change works and update the current diff later.

sthaha · 2023-07-11T06:18:53Z

I have refactor PR that "cleans up" the setup script - #31
It would be nice to get your thoughts @SamYuan1990 @jiere

jiere · 2023-07-11T07:44:26Z

I have refactor PR that "cleans up" the setup script - #31 It would be nice to get your thoughts @SamYuan1990 @jiere

@sthaha , I like your idea to adjust the directory hierarchy and also code location such as moving code from common.sh to main.sh. Please hold on and give me some time to refine current PR based on above comments, then you can move on accordingly, thanks.

jiere · 2023-07-11T09:23:01Z

sed is a common way to replace the value. :-) if there any other good suggestion, please let me know.

Just tried, sed -i worked well :-)

vprashar2929 · 2023-07-11T09:47:43Z

sed is a common way to replace the value. :-) if there any other good suggestion, please let me know.

Just tried, sed -i worked well :-)

AFAIK sed has different implementations on Linux and macOS. On Linux, it seems to work fine but I have faced an issue on macOS. A related PR on this

SamYuan1990 · 2023-07-11T12:24:11Z

This PR has no conflict with my earlier raised sustainable-computing-io/kepler-action#50, in the mid-term or long-term, we should support more OS types of runners other than current Ubuntu. Current PR is discussing another topic, for any runner OS, shall we local-dev-cluster repo code take care of those prerequisite package installation stuffs? In my point of view, no matter which OS we are running on the runner, such package installation stuffs should be taken care of by the runner admin, but not our code. Take Github-hosted runner as example, Microsoft/Github guys are working hard to maintain their runners, even create repo for it, and also setup related policies. Today, we can use functions such as action/setup-go in our GHA workflow yml just because they have written code for it, if no such code maintained by the runner admin, do we need to add the golong installation code in our local-dev-cluster repo also?

@jiere .... as mentioned in sustainable-computing-io/kepler-action#50 (comment) if we just considering with GHA, even if self host instance, then it works. but for other CI pipelines, as travis ... are those binaries are default settings? I am not sure...

SamYuan1990 · 2023-07-11T12:26:06Z

@rootfs , what do you think of , do we need considering with other CI tools and keep kubectl and kind binary install today?

SamYuan1990 · 2023-07-11T12:30:19Z

summary for today's status. Please correct me for any mistake.

We still open discussion for flexibility to avoid port conflicts. Hence if further considering running on self host agent, can use job_number as port number to avoid avoid port conflicts. @jiere , please let me know if I miss that changes.
We sill open discussion for different CI pipeline usage. Hence for other pipeline, which don't install kubectl or kind binary by default.

marceloamaral · 2023-07-11T18:40:14Z

README.md


 ## Start up
-1. Modify kind [config](./kind/manifests/kind.yml) to make sure `extraMounts:` cover the linux header and BCC.
+1. Modify kind [config](./kind/kind.yml) to make sure `extraMounts:` cover the linux header and BCC.
 2. Export `CLUSTER_PROVIDER` env variable:
 ```
 export CLUSTER_PROVIDER=kind/microshift


We should change to the or symbol: export CLUSTER_PROVIDER=[kind|microshift] is better

It seems that export grammar does not support such writings, let me change as:
export CLUSTER_PROVIDER=kind or export CLUSTER_PROVIDER=microshift, is this acceptable for you? @marceloamaral

marceloamaral · 2023-07-11T18:46:36Z

I am open to utilizing pre-defined GHA (GitHub Actions) as long as we ensure consistency with the local cluster deployment. It is important for the developer to have a similar environment to test before pushing a commit.

Ultimately, we should use what makes the most sense for us.

SamYuan1990 · 2023-07-12T11:45:47Z

As for today, I keep my standard to review this PR as the same for https://github.com/sustainable-computing-io/local-dev-cluster/pull/9/files if it doesn't break CI, then I can accept with merge it. But for further consideration, I suppose this PR or parts of this PR reduced our robust and flexibility. And in further if we considering with sustainable-computing-io/kepler-action#50 (comment) we have to add them back?(maybe not code back but feature back)

hence recall myself, I prefer to merge this PR base on what we have today.
I will keep this PR open for review for this week.(@jiere , @rootfs , @vprashar2929 , @marceloamaral )
for port number we can fix it once we have a self hosted instance.

vprashar2929 · 2023-07-12T16:56:55Z

kind/kind.sh

 function _prepare_config() {
-    echo "Building manifests..."
+     # replace registry name in kind.yml
+     sed -i "s/kind-registry/${KIND_REGISTRY_NAME}/g" ${KIND_DIR}/kind.yml


@jiere sed -i will not work on mac. The script fails with this change on mac

Yes, I did see @sthaha 's suggestion above, let me change a new version and force-push diff again.

sthaha · 2023-07-13T06:14:40Z

kind/kind.sh

-function _fetch_kind() {
-    mkdir -p "${KIND_DIR}"
-    KIND="${KIND_DIR}"/.kind
-    if [ -f "$KIND" ]; then
-        current_kind_version=$($KIND --version | awk '{print $3}')
-    fi
-    if [[ $current_kind_version != $KIND_VERSION ]]; then
-        echo "Downloading kind v$KIND_VERSION"
-        if [[ "$OSTYPE" == "darwin"* ]]; then
-            curl -LSs https://github.com/kubernetes-sigs/kind/releases/download/v$KIND_VERSION/kind-darwin-${ARCH} -o "$KIND"
-        else
-            curl -LSs https://github.com/kubernetes-sigs/kind/releases/download/v$KIND_VERSION/kind-linux-${ARCH} -o "$KIND"
-        fi
-        chmod +x "$KIND"
-    fi
-}
-


imho, we shouldn't remove this rather use the approach in #31 to validate if kind exists and the version is as expected.

@sthaha , the key idea for this PR is not statically or ENV-based define the kind version, we just need to transfer the kind installation effort to runner admin, just as Github-hosted runners do. For developers, he/she needs to do one-shot installation of kind and kubectl when he setup his/her test machine at the very beginning.

I agree about the installation part, but the validation for the versions must be performed in the script to ensure everyone stays on the same version. We can also add --no-validate or --ci flag to suppress unnecessary validations in CI.

sthaha · 2023-07-13T06:16:30Z

kind/kind.sh

    _prepare_config
    _setup_kind
+    _run_kind_registry


I think the function should adhere to single-responsiblity principle and only setup kind, not the registry. Use a different function (like it was earlier) to setup registry.

Yeah, the reason why I added _run_kind_registry into kind_up is just to let the kind code in current common.sh align with behavior of microshift code.
For current diff, _setup_kind and _run_kind_registry are two steps for _kind_up, each has single-responsibility already; In common.sh, both kind and microshift cluster setup will have two similar steps: _xxx_up and _wait_for_clusterReady, not like current 3 steps for kind and 2 steps for microshift :-)

Speaking of which could this change please wait for #31 which does a major overhaul of the scripts merge?

sthaha · 2023-07-13T06:19:38Z

verify.sh

+    kubectl cluster-info
+    if [ "$?" != 0 ]; then


Consider these 2 alternatives

My preference - use && ||

kubectl cluster-info || { echo "failed .. "; return 1; }

in #31 I have a generic die method that exits 1

No need to use if [

if ! kubectl cluster-info ; then ... fi

Cool, I like the first one :-) Thanks.
The initial reason for me to change here is just removing the original console warning.
Checked your diff in #31, since current diff is just subset of yours, let me back out my change here and leave it to yours :-)

Alternatively, since #31 is approved, it would be great if you could rebase the major changes on top of #31.

sthaha · 2023-07-14T00:39:59Z

kind/kind.sh

-    if [ -f "$KIND" ]; then
-        current_kind_version=$($KIND --version | awk '{print $3}')
-    fi
-    if [[ $current_kind_version != $KIND_VERSION ]]; then


can we move this validation to verify.sh tools instead? This allows everyone to validate if the versions of the binaries in path are good!

rootfs · 2023-07-14T12:18:35Z

CI failed

./main.sh: line 74: CONFIG_OUT_DIR: unbound variable
Error: Process completed with exit code 1.

jiere · 2023-07-14T14:34:19Z

CI failed

./main.sh: line 74: CONFIG_OUT_DIR: unbound variable
Error: Process completed with exit code 1.

Missed one line deletion... My bad, let me rebase again...

1. For `kubectl` and `kind`, using Github-hosted runners' provisioned version. 2. Simplify code logic, remove some unnecessary ENV options. Signed-off-by: Jie Ren <[email protected]>

SamYuan1990 requested a review from marceloamaral July 9, 2023 03:12

rootfs requested a review from SamYuan1990 July 9, 2023 13:40

SamYuan1990 reviewed Jul 9, 2023

View reviewed changes

SamYuan1990 changed the title ~~Fix issue #26: Cleanup and simplify code logic~~ [Under discussion, don't merge]Fix issue #26: Cleanup and simplify code logic Jul 10, 2023

vprashar2929 mentioned this pull request Jul 11, 2023

refactor(main): improve maintainability #31

Merged

jiere force-pushed the cleanup branch from 0acd7e5 to edd533a Compare July 11, 2023 10:18

marceloamaral reviewed Jul 11, 2023

View reviewed changes

SamYuan1990 approved these changes Jul 12, 2023

View reviewed changes

SamYuan1990 changed the title ~~[Under discussion, don't merge]Fix issue #26: Cleanup and simplify code logic~~ Fix issue #26: Cleanup and simplify code logic Jul 12, 2023

vprashar2929 requested changes Jul 12, 2023

View reviewed changes

sthaha reviewed Jul 13, 2023

View reviewed changes

jiere force-pushed the cleanup branch from edd533a to c96bace Compare July 13, 2023 12:45

sthaha reviewed Jul 14, 2023

View reviewed changes

jiere force-pushed the cleanup branch from c96bace to 7532316 Compare July 14, 2023 06:40

sthaha mentioned this pull request Jul 14, 2023

Enable shellcheck in github worflow #33

Merged

rootfs approved these changes Jul 14, 2023

View reviewed changes

jiere force-pushed the cleanup branch from 7532316 to ad20f8d Compare July 14, 2023 15:20

Cleanup and simplify code logic

c96c2bc

1. For `kubectl` and `kind`, using Github-hosted runners' provisioned version. 2. Simplify code logic, remove some unnecessary ENV options. Signed-off-by: Jie Ren <[email protected]>

jiere force-pushed the cleanup branch from ad20f8d to c96c2bc Compare July 14, 2023 21:47

rootfs merged commit 68ae9c5 into sustainable-computing-io:main Jul 14, 2023

jiere deleted the cleanup branch July 14, 2023 22:14

Fix issue #26: Cleanup and simplify code logic #28

Fix issue #26: Cleanup and simplify code logic #28

Conversation

jiere commented Jul 8, 2023 • edited Loading

jiere commented Jul 8, 2023

SamYuan1990 commented Jul 9, 2023

jiere commented Jul 9, 2023

rootfs commented Jul 9, 2023

SamYuan1990 commented Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vprashar2929 commented Jul 10, 2023

jiere commented Jul 10, 2023 • edited Loading

SamYuan1990 commented Jul 10, 2023

SamYuan1990 commented Jul 10, 2023

jiere commented Jul 11, 2023

sthaha commented Jul 11, 2023

jiere commented Jul 11, 2023

jiere commented Jul 11, 2023

vprashar2929 commented Jul 11, 2023

SamYuan1990 commented Jul 11, 2023 • edited Loading

SamYuan1990 commented Jul 11, 2023

SamYuan1990 commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marceloamaral commented Jul 11, 2023

SamYuan1990 commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiere Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootfs commented Jul 14, 2023

jiere commented Jul 14, 2023

jiere commented Jul 8, 2023 •

edited

Loading

SamYuan1990 commented Jul 9, 2023 •

edited

Loading

jiere commented Jul 10, 2023 •

edited

Loading

SamYuan1990 commented Jul 11, 2023 •

edited

Loading

sthaha Jul 14, 2023 •

edited

Loading

jiere Jul 13, 2023 •

edited

Loading