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

local-dev-cluster repo is not correctly cloned when running "make cluster-up" #721

Closed
jiere opened this issue Jun 3, 2023 · 28 comments
Closed

Comments

@jiere
Copy link
Collaborator

jiere commented Jun 3, 2023

Describe the bug
When running "make cluster-up", the local-dev-cluster repo is not correctly cloned.
This may be due to change here

To Reproduce
Steps to reproduce the behavior:

  1. Run "make cluster-up" or just run "git clone -b v0.0.0 https://github.com/sustainable-computing-io/local-dev-cluster.git --depth=1"

  2. See error msg below:

Note: switching to 'cc8cc366a0f86c891ff867aca914a95af535e2a6'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

git switch -c

Or undo this operation with:

git switch -

Turn off this advice by setting config variable advice.detachedHead to false

$ cd local-dev-cluster/
[jie@jie-dev local-dev-cluster]$ git branch

  • (no branch)
  1. Check code in local-dev-cluster directory, both main.sh and kind/common.sh are not up-to-date with the repo.

Expected behavior
Correctly git clone the local-dev-cluster and do not break the new feature for #718

Additional context
when remove the "-b v0.0.0" words from the git clone, no issue found yet.

@jiere
Copy link
Collaborator Author

jiere commented Jun 3, 2023

@SamYuan1990

@SamYuan1990
Copy link
Collaborator

en? https://github.com/sustainable-computing-io/kepler/actions/runs/5134947333/jobs/9239675792
is not breaking? as check out with specific version is git cli feature?

@SamYuan1990
Copy link
Collaborator

SamYuan1990 commented Jun 4, 2023

or I am confusing as https://github.com/sustainable-computing-io/kepler/actions/runs/5134947333/jobs/9239675792#step:4:6 as the same git cli command has different result at your local or...? then I will request @jiere provides more details on your local environment.

@SamYuan1990
Copy link
Collaborator

git clone -h
usage: git clone [<options>] [--] <repo> [<dir>]

    -v, --verbose         be more verbose
    -q, --quiet           be more quiet
    --progress            force progress reporting
    -n, --no-checkout     don't create a checkout
    --bare                create a bare repository
    --mirror              create a mirror repository (implies bare)
    -l, --local           to clone from a local repository
    --no-hardlinks        don't use local hardlinks, always copy
    -s, --shared          setup as shared repository
    --recursive[=<pathspec>]
                          initialize submodules in the clone
    --recurse-submodules[=<pathspec>]
                          initialize submodules in the clone
    -j, --jobs <n>        number of submodules cloned in parallel
    --template <template-directory>
                          directory from which templates will be used
    --reference <repo>    reference repository
    --reference-if-able <repo>
                          reference repository
    --dissociate          use --reference only while cloning
    -o, --origin <name>   use <name> instead of 'origin' to track upstream
    -b, --branch <branch>
                          checkout <branch> instead of the remote's HEAD
    -u, --upload-pack <path>
                          path to git-upload-pack on the remote
    --depth <depth>       create a shallow clone of that depth
    --shallow-since <time>
                          create a shallow clone since a specific time
    --shallow-exclude <revision>
                          deepen history of shallow clone, excluding rev
    --single-branch       clone only one branch, HEAD or --branch
    --no-tags             don't clone any tags, and make later fetches not to follow them
    --shallow-submodules  any cloned submodules will be shallow
    --separate-git-dir <gitdir>
                          separate git dir from working tree
    -c, --config <key=value>
                          set config inside the new repository
    --server-option <server-specific>
                          option to transmit
    -4, --ipv4            use IPv4 addresses only
    -6, --ipv6            use IPv6 addresses only
    --filter <args>       object filtering
    --remote-submodules   any cloned submodules will use their remote-tracking branch

@SamYuan1990
Copy link
Collaborator

yuanyi@yuanyideMacBook-Pro tmp % git clone -b v0.0.0 https://github.com/sustainable-computing-io/local-dev-cluster.git --depth=1
Cloning into 'local-dev-cluster'...
remote: Enumerating objects: 14, done.
remote: Counting objects: 100% (14/14), done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 14 (delta 1), reused 8 (delta 0), pack-reused 0
Unpacking objects: 100% (14/14), done.
Note: switching to 'cc8cc366a0f86c891ff867aca914a95af535e2a6'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

yuanyi@yuanyideMacBook-Pro tmp % ls
demo			java-gm			local-dev-cluster	react-express-fullstack	tape
yuanyi@yuanyideMacBook-Pro tmp % cd local-dev-cluster 
yuanyi@yuanyideMacBook-Pro local-dev-cluster % ls
LICENSE		README.md	kind		main.sh
yuanyi@yuanyideMacBook-Pro local-dev-cluster % git status
Not currently on any branch.
nothing to commit, working tree clean
yuanyi@yuanyideMacBook-Pro local-dev-cluster % git log -1
commit cc8cc366a0f86c891ff867aca914a95af535e2a6 (grafted, HEAD, tag: v0.0.0)
Author: Huamin Chen <[email protected]>
Date:   Tue May 23 08:04:53 2023 -0400

    Merge pull request #9 from vprashar2929/fix
    
    Fix flaky common.sh

@jiere , I can't reproduce at my local.... let me know what's your specific case.

@jiere
Copy link
Collaborator Author

jiere commented Jun 4, 2023

@SamYuan1990 , actually you have reproduced:)
Please check the code content of your cloned repo, both main.sh and kind/common.sh do not include the change in local-dev-cluster repo's PR#10.

@jiere
Copy link
Collaborator Author

jiere commented Jun 4, 2023

en? https://github.com/sustainable-computing-io/kepler/actions/runs/5134947333/jobs/9239675792 is not breaking? as check out with specific version is git cli feature?

The current job is not broken because it only checks if the make cluster-up result is OK, the current content of cloned local-dev-cluster won't break it.
However, if other cases need to leverage the further actions there. i.e. run ./main.sh down to tear down the kind cluster, will definitely fail, because the current cloned main.sh still the old one and does not support any options such as up and down.

@jiere
Copy link
Collaborator Author

jiere commented Jun 4, 2023

I am just curious about the current usage here, why hardcode to be -b v0.0.0?

I also checked the branch/tag of local-dev-cluster repo
$ git branch -a
main
remotes/origin/HEAD -> origin/main
remotes/origin/main
$ git tag
v0.0.0
$ git log
......
commit cc8cc366a0f86c891ff867aca914a95af535e2a6 (tag: v0.0.0)
Merge: 74fd492 37091d3
Author: Huamin Chen [email protected]
Date: Tue May 23 08:04:53 2023 -0400

Merge pull request #9 from vprashar2929/fix

Fix flaky common.sh

......

As above git log shown, the v0.0.0 tag was on 05/23 commit, so all the commits later than that time was missing when using "git clone -b v0.0.0"...

@SamYuan1990
Copy link
Collaborator

@SamYuan1990 , actually you have reproduced:) Please check the code content of your cloned repo, both main.sh and kind/common.sh do not include the change in local-dev-cluster repo's PR#10.

yes I know that. well it's different with git clone -b is an issue.

@SamYuan1990
Copy link
Collaborator

@jiere , TL;DR the check out with the tag version v0.0.0 with the commit id is correct one by design.

Here is some back ground with different options through our community and I try to make a trade off for now as https://github.com/sustainable-computing-io/kepler/blob/main/enhancements/CICDv1.md +@rootfs here.
or as you mentioned in sustainable-computing-io/local-dev-cluster#10, Once I blocked it by make kepler consume with tag 1st.

In recently, there are lots of voices about versioning tagging etc.
sustainable-computing-io/kepler-helm-chart#15 (comment)
sustainable-computing-io/kepler-helm-chart#28 (comment)
from +@bradmccoydev, even if not using tag but specific commit hash, or some requirements from OpenSSF point of view.

So far I would like to use tag.

  • instead of nothing, honestly, in the long time ago, we use nothing.
    As we don't have lots of requirement to update with local-dev-cluster or efforts to upgrade local-dev-cluster.
    But as we have new requirements and features need to be updated in repo local-dev-cluster. Check out with a tag here help's us points to a specific version to avoid our changes breaks CI.

  • instead of commit hash, I don't want to use a specific hash as magic. in short, either we don't have a case to overwrite a tag or in personally hope to see the case, even if it breaks OpenSSF? A commit tag of local-dev-cluster means how external invokes it. For example kepler in make script or kepler-action. Your request as cluster-down is a breaking change from v0.0.0 as in v0.0.1 we will add a new parameter.

@jiere , maybe a solution as a trade off is that if we have local-cluster folder, then we skip the git clone. But anyway which is different with git clone -b exists with 1 in bash command, as the make scripts implements in error.

@jiere ,@rootfs , I am not considering with OpenSSF as I mentioned at #717 (comment), but open for any discussion.
For me, I will review from:

  1. Don't break CI.
  2. Benefits for our life.

@bradmccoydev
Copy link
Member

My opinion on this is to do a tag for releases for each repo, and folks can choose to either use the main branch, the tag, or sha. Doing releases provides the options eg 0.0.1, 0.0.2, 0.0.3, etc to stay on that code or rollback if they are not ready for the new code. It doesn't hurt anyone, and then if people don't want to reference the tag, they can simply point to the main branch. Pointing to the main branch is fine in scenarios but giving other people the option to reference versions is an easy thing to do. Happy to talk about it in a meeting if you like, I hope that makes sense

@SamYuan1990
Copy link
Collaborator

My opinion on this is to do a tag for releases for each repo, and folks can choose to either use the main branch, the tag, or sha. Doing releases provides the options eg 0.0.1, 0.0.2, 0.0.3, etc to stay on that code or rollback if they are not ready for the new code. It doesn't hurt anyone, and then if people don't want to reference the tag, they can simply point to the main branch. Pointing to the main branch is fine in scenarios but giving other people the option to reference versions is an easy thing to do. Happy to talk about it in a meeting if you like, I hope that makes sense

I do agree.

SamYuan1990 added a commit to SamYuan1990/kepler that referenced this issue Jun 4, 2023
@rootfs rootfs closed this as completed in 1563764 Jun 5, 2023
@jiere
Copy link
Collaborator Author

jiere commented Jun 5, 2023

@SamYuan1990 and @bradmccoydev , I do like the idea of tag, what I am confusing is that why we hardcode the tag value to v0.0.0 in kepler's Makefile? Since most Kepler developers use local-dev-cluster as their dev setup, so kepler repo is the most important consumer of local-dev-cluster, right?
Per my understanding, the fix here won't break any CI also. I do agree that local-dev-cluster itself will not be changed frequently, but if we really need a tag in kepler Makefile's git clone operations, why not use a newer tag? Otherwise, for consumers like me and @vprashar2929 needs to manually(or hardcode in scripts) switch the cloned repo to main(or other tags) to catch up with our expected code each time...

BTW, could you also share the method to switch to main branch or other tags? I just tried failed like this:

$ git branch
** (no branch)
$ git log -1
commit cc8cc366a0f86c891ff867aca914a95af535e2a6 (grafted, HEAD, tag: v0.0.0)
Author: Huamin Chen <[email protected]>
Date:   Tue May 23 08:04:53 2023 -0400

    Merge pull request #9 from vprashar2929/fix

    Fix flaky common.sh
$ git switch -c main
Switched to a new branch 'main'
$ git branch -a
** main
$ git log
commit cc8cc366a0f86c891ff867aca914a95af535e2a6 (grafted, HEAD -> main, tag: v0.0.0)
Author: Huamin Chen <[email protected]>
Date:   Tue May 23 08:04:53 2023 -0400

    Merge pull request #9 from vprashar2929/fix

    Fix flaky common.sh
$ git pull
Already up to date.

@rootfs
Copy link
Contributor

rootfs commented Jun 5, 2023

@jiere good point.
@SamYuan1990 @bradmccoydev how about creating release for local-dev-cluster the same way as helm chart? Each time the helm chart gets an update, the reversion bumps up as well. In the same manner, the kepler repo will use the newer release from local-dev-cluster to keep up with the changes.

@jiere
Copy link
Collaborator Author

jiere commented Jun 5, 2023

Thanks @rootfs for your suggestion, lgtm:)
folks, I also just found that current git clone usage has a weak point:
https://github.com/sustainable-computing-io/kepler/blob/main/hack/cluster-up.sh#LL24C88-L24C97,
if we use "--depth=1", we miss the original/main branch info, cannot switch back anymore;
even though we do not use "--depth=1", the switch back to main should be like this (need altogether three git commands...):

$ git switch -c main
$ git branch --set-upstream-to=origin/main main
$ git pull

@SamYuan1990
Copy link
Collaborator

@jiere good point. @SamYuan1990 @bradmccoydev how about creating release for local-dev-cluster the same way as helm chart? Each time the helm chart gets an update, the reversion bumps up as well. In the same manner, the kepler repo will use the newer release from local-dev-cluster to keep up with the changes.

1st of all, I don't want to force binding, unless CI usage.
Here is my point, in kepler's repo, we allow user to use their own local-dev-cluster. We just use a stable version as CI!
@jiere , you can make your own fork locally for local development.
Or considering some cases below:

  • Don't break CI.
  • We may have new feature for local-dev-cluster.
  • We need some how development for those new features in repo local-dev-cluster.
  • We still need make script adopt to use new features in local-dev-cluster, as cluster down.
  • For each developer, the cgroup or ebpf settings may different according to linux BM/VM/OS, in fact it's allowed local version of local-dev-cluster anyway due to the path issue.

Hence, if you want to use cluster down, you can enable at your local or commit a PR to default branch.
But before local-dev-cluster version bump up to v0.0.1, or some how, before release 0.6 in kepler, this cluster down feature is something as experiment. That's for release management point of view:

For the tag and commit out of the tag:
I don't like to trace the time line as audit... but as the time flow, the pr adding a new parameter as up/down. which I blocked it and tag without it.
The PR version as I reviewed.
sustainable-computing-io/local-dev-cluster@2604794

The comments out as I blocking for tag, as the new parameter been treated as a breaking change.
sustainable-computing-io/local-dev-cluster#10 (review)
And I created tags to make kepler consume local-dev-cluster with stable version in CI.
That's the reason why this PR out of v0.0.0 scope.

Honestly, I know there is an other option as the second commit as sustainable-computing-io/local-dev-cluster@232fce7
But... we always need a switch for tag.

As a summary,
We can't avoid local version of local-cluster-dev for developer usage. As either path issue or experiment features. We just keep CI works with a stable version.

@SamYuan1990
Copy link
Collaborator

I feel like @jiere really want local-dev-cluster with 1st parameter control cluster up or down. like

./main.sh up/down

but in my point of view, the 1st parameter maybe KIND or mircoshift(#182) even if for local dev cluster usage.
hence there is another chance for us as below

./main.sh KIND/microshift up/down

which is not finalized...
@rootfs , could you please help push #182 ?

@vprashar2929
Copy link
Collaborator

Absolutely @SamYuan1990 I agree with the notion of keeping CI at a stable version as there will always be scope for improvement/changes in local-dev-cluster and it will be hard to keep track of updating its usage dependencies everywhere in future

@jiere
Copy link
Collaborator Author

jiere commented Jun 5, 2023

haha, actually I am preparing scripts for this PR #711 , since they are automation, we must guarantee that the cluster is setup/teardown properly among different cases, this is why I like this make cluster-up/cluster-down command :D

Anyway, I think I totally got your point, Sam. I will try to modify my local scripts to mitigate this. But last thing I want to repeat, could you kindly remove the "--depth=1" then?

@jiere
Copy link
Collaborator Author

jiere commented Jun 5, 2023

but in my point of view, the 1st parameter maybe KIND or mircoshift(#182) even if for local dev cluster usage

May I ask an easy question: is microshift free for use or not? It sounds like an "Edge Computing" version of OpenShift, as we all know that, OpenShift needs license...

@SamYuan1990
Copy link
Collaborator

SamYuan1990 commented Jun 5, 2023

haha, actually I am preparing scripts for this PR #711 , since they are automation, we must guarantee that the cluster is setup/teardown properly among different cases, this is why I like this make cluster-up/cluster-down command :D

Anyway, I think I totally got your point, Sam. I will try to modify my local scripts to mitigate this. But last thing I want to repeat, could you kindly remove the "--depth=1" then?

For #711 , please take a look at:
#711 (review)

For --depth=1 as a result no for now, just checkout a specific branch with --depth=1 will save a lot of time in CI as it just checkout a single commit.(for performance consideration)

an suggestion is made in #711

@SamYuan1990
Copy link
Collaborator

but in my point of view, the 1st parameter maybe KIND or mircoshift(#182) even if for local dev cluster usage

May I ask an easy question: is microshift free for use or not? It sounds like an "Edge Computing" version of OpenShift, as we all know that, OpenShift needs license...

+@rootfs

@vprashar2929
Copy link
Collaborator

May I ask an easy question: is microshift free for use or not? It sounds like an "Edge Computing" version of OpenShift, as we all know that, OpenShift needs license...

I use Microshift for CI purposes and its free to use.

@rootfs
Copy link
Contributor

rootfs commented Jun 5, 2023

@vprashar2929 can you create a microshift env for CI? Thanks

@vprashar2929
Copy link
Collaborator

Sure I am happy to work on this🙂. I suspect we use these steps to deploy Kepler on OCP or we wish something along similar lines with our current integration tests?

@rootfs
Copy link
Contributor

rootfs commented Jun 7, 2023

@vprashar2929 manifests will be a good start. Thank you!
@husky-parul this could be reused in operator

@SamYuan1990
Copy link
Collaborator

@vprashar2929 manifests will be a good start. Thank you! @husky-parul this could be reused in operator

@rootfs , @vprashar2929 , may suggestion is to add shell script into local-dev-cluster
as kepler-action invokes local-dev-cluster to start up k8s cluster for test.
then... kepler, operator, helm chart can cover the test case by invoke kepler-action.

@vprashar2929
Copy link
Collaborator

Sure adding microshift cluster start-up/down in local-dev-cluster sounds good.

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

No branches or pull requests

5 participants