Fix stack name when manifest at .okteto folder #2742

teresaromero · 2022-06-01T10:30:54Z

Signed-off-by: Teresa Romero teresa@okteto.com

Fixes #2661

Proposed changes

when docker-compose.yaml is under an .okteto folder at the repository, the dev environment name was being used was okteto. - fixed the parsing for directories
if dev environment was okteto destroy and redeploy the components
old okteto dev environment will be left empty, used can activly destroy it by ui or cli destroy --name

How to reproduce

clone https://github.com/okteto/compose-getting-started
move the docer-compose to .okteto folder inside the repo
run okteto deploy with actual cli
run okteto deploy with the build of this branch

codecov · 2022-06-01T10:31:28Z

Codecov Report

Merging #2742 (0e65641) into master (4e10434) will increase coverage by 0.07%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2742      +/-   ##
==========================================
+ Coverage   32.10%   32.18%   +0.07%     
==========================================
  Files         158      158              
  Lines       18580    18592      +12     
==========================================
+ Hits         5966     5984      +18     
+ Misses      11909    11896      -13     
- Partials      705      712       +7

Impacted Files	Coverage Δ
pkg/cmd/stack/deploy.go	`23.26% <42.85%> (+2.84%)`	⬆️
pkg/model/utils.go	`68.93% <100.00%> (+0.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e10434...0e65641. Read the comment docs.

jmacelroy · 2022-06-02T01:30:29Z

cmd/deploy/deploy.go

@@ -152,13 +152,18 @@ func Deploy(ctx context.Context) *cobra.Command {
 				}
 			}

+			// when name is an option - this overrides the manifest or inferred name
+			if options.Name != "" {
+				os.Setenv(model.OktetoNameEnvVar, options.Name)


This seems like a very indirect way of setting this and is quite brittle. We've effectively set a global variable, in env, in order to make a decision later on. What are all of the ways this env var get set and how can we track them? Is there a more direct approach we can take that eliminates this?

It seems like a better, and less brittle way, would be to set the name as a config option and somehow passing it into the code that will eventually do the deploy directly.

yup, this was actually not needed to be fixed :/ i've updated the scope for the PR due to the comments at the issue #2661

ifbyol · 2022-06-02T14:51:40Z

Note to bear in mind. This could be considered as a non-backward compatible change. If I already executed deployed an application with current version of the CLI, I would have a dev environment called okteto. If I update the CLI after releasing this change and do a redeploy using the same command, now, the name of the dev environment would be different (okteto folder's parent folder) and I would deploy the entire dev environment instead of redeploying the existing one and it might end up with some k8s object collision

I don't know how frequent could be this case, but commented just to take into account

cc @jmacelroy @pchico83

teresaromero · 2022-06-03T07:09:57Z

Note to bear in mind. This could be considered as a non-backward compatible change. If I already executed deployed an application with current version of the CLI, I would have a dev environment called okteto. If I update the CLI after releasing this change and do a redeploy using the same command, now, the name of the dev environment would be different (okteto folder's parent folder) and I would deploy the entire dev environment instead of redeploying the existing one and it might end up with some k8s object collision

I don't know how frequent could be this case, but commented just to take into account

cc @jmacelroy @pchico83

Running okteto deploy with current -> deploys the okteto dev environment
Running again okteto deploy with changes -> creates an empty dev environment with the correct name (inferred by the folder) but there are some warnings and the k8s components are not "updated" to be at the new dev environment

CLI logs:


❯ okteto deploy
 i  Using teresa @ okteto.teresa.dev.okteto.net as context
 i  Building image for service 'vote'...
 i  To rebuild your image manually run 'okteto build vote' or 'okteto deploy --build'
 i  Building image for service 'vote'
[+] Building 22.9s (11/11) FINISHED
 ✓  Image for service 'vote' pushed to registry: registry.teresa.dev.okteto.net/teresa/okteto-vote:okteto-with-volume-mounts
 i  Deploying compose
 ✓  Volume 'redis' created
 ✓  Service 'vote' created
 ✓  Service 'redis' created
 ✓  Compose 'okteto' successfully deployed
 ✓  Development environment 'okteto' successfully deployed
 i  Run 'okteto up' to activate your development container

❯ ok deploy
 i  Using teresa @ okteto.teresa.dev.okteto.net as context
 i  Images were already built. To rebuild your images run 'okteto build' or 'okteto deploy --build'
 ✓  Endpoint 'vote' created
 !  skipping creation of volume 'redis' due to name collision with volume in stack 'okteto'
 !  skipping deploy of deployment 'vote' due to name collision with deployment in stack 'okteto'
 !  skipping deploy of statefulset 'redis' due to name collision with statefulset in stack 'okteto'
 ✓  Compose 'compose-getting-started' successfully deployed

ifbyol · 2022-06-03T07:43:00Z

@teresaromero what would happen if after that I execute okteto destroy?

teresaromero · 2022-06-03T08:09:34Z

@teresaromero what would happen if after that I execute okteto destroy?

when using the current one, the okteto dev environment is deleted, but the changes, deletes the new dev environment, okteto still is there

Should we add some logic so the components change their labels and dev environment name to be swaped?

pchico83 · 2022-06-03T08:16:56Z

@teresaromero what would happen if after that I execute okteto destroy?

when using the current one, the okteto dev environment is deleted, but the changes, deletes the new dev environment, okteto still is there

Should we add some logic so the components change their labels and dev environment name to be swaped?

I would add logic to, in case we are in .okteto, replace this warning ! skipping by actually modifying the component if its dev environment name was okteto.

@ifbyol great catch!

teresaromero · 2022-06-03T13:09:30Z

@teresaromero what would happen if after that I execute okteto destroy?

when using the current one, the okteto dev environment is deleted, but the changes, deletes the new dev environment, okteto still is there
Should we add some logic so the components change their labels and dev environment name to be swaped?

I would add logic to, in case we are in .okteto, replace this warning ! skipping by actually modifying the component if its dev environment name was okteto.

@ifbyol great catch!

I've been going tthough the code to make this modification, we have to change all components ( deployments, sfs, jobs, volumes, etc...) ?

teresaromero · 2022-06-06T14:30:11Z

I've tried to add some logic to swap the stack names in case the old one was okteto but i get an error from k8s

Error updating deployment of service 'vote': Deployment.apps "vote" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"stack.okteto.com/name":"compose-getting-started", "stack.okteto.com/service":"vote"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

as the labels are inmutable, should then display a warning message so the user can destroy the okteto dev environment and start again?

cc: @pchico83 @ifbyol

pchico83 · 2022-06-06T19:56:04Z

I've tried to add some logic to swap the stack names in case the old one was okteto but i get an error from k8s
Error updating deployment of service 'vote': Deployment.apps "vote" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"stack.okteto.com/name":"compose-getting-started", "stack.okteto.com/service":"vote"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable
as the labels are inmutable, should then display a warning message so the user can destroy the okteto dev environment and start again?

cc: @pchico83 @ifbyol

@teresaromero I would automatically destroy + deploy the resources, and keep only the volumes. That way, the operation is transparent to the user. We already do something similar when a statefulset cannot be edited

teresaromero · 2022-06-07T09:22:18Z

@ifbyol @jLopezbarb can you check the latest changes that involve destroy de compontent and redeploy with new labels? thanks

teresaromero · 2022-06-07T14:42:31Z

edit description with instructions on how to reproduce the change

ifbyol · 2022-06-08T07:49:18Z

pkg/cmd/stack/deploy.go

+		if err := deployments.Destroy(ctx, old.Name, old.Namespace, c); err != nil {
+			return false, fmt.Errorf("error updating deployment of service '%s': %s", svcName, err.Error())
+		}
+		if _, err := deployments.Deploy(ctx, d, c); err != nil {
+			return false, fmt.Errorf("error updating deployment of service '%s': %s", svcName, err.Error())
+		}


This is odd, why if it failed because a temporal error redeploying? Any error in a redeploy would imply to destroy a deploy again even if it is not necessary, right?

you are right, in case of statefulsets we make this only when the error is not "Forbidden", do you think we should control other types of errorS?

@ifbyol i've reverted the change here and added an exception prior to deploying for first time to check if is the exception, if yes, then we destroy and redeploy

ok, I think it looks good now.

Maybe we can include a comment before the if to explain why this piece of code is needed? If not, in a few months we won't remember why we do that check

i've added a comment with a link to this PR and some context

Signed-off-by: Teresa Romero <teresa@okteto.com>

ifbyol · 2022-06-09T07:57:01Z

pkg/cmd/stack/deploy.go

 			return false, fmt.Errorf("skipping deploy of deployment '%s' due to name collision with deployment in stack '%s'", svcName, old.Labels[model.StackNameLabel])
 		}
 		if v, ok := old.Labels[model.DeployedByLabel]; ok {
+			if old.Labels[model.StackNameLabel] == "okteto" {
+				d.Labels[model.DeployedByLabel] = s.Name
+			}


Don't you need an else here? If you set the deployed-by label to the new name, the line 498 will overwrite it with the value coming from the old deployment, right?

Or moving the line 498 before doing the okteto check would be enough

Signed-off-by: Teresa Romero <teresa@okteto.com>

jLopezbarb · 2022-06-09T13:38:52Z

Can we add some unit test with the scenario of having an .okteto folder

Signed-off-by: Teresa Romero <teresa@okteto.com>

teresaromero · 2022-06-10T09:06:14Z

@maroshii i've added both 2 labels: bugfix and breaking change... its definatly a bug fix as the users which use an .okteto folder for their compose and stacks will now have the dev environment name fixed, but at the same time, if someone had a dev environment deployed with prev version of cli, now the old one will be empty and they will se a new one with the correct name, not sure if this is a breking change, or "not that breaking" change.. but is a change :/

EDIT: on i get an error that only 1 so... i will go with bug fix as we have logic in place to dont break

teresaromero requested a review from a team June 1, 2022 10:30

jmacelroy reviewed Jun 2, 2022

View reviewed changes

teresaromero force-pushed the tere/2661-fix-name-stack branch from 5e07b4d to 943ee13 Compare June 2, 2022 08:32

teresaromero changed the title ~~Override stack name if option is not empty~~ Fix stack name when manifest at .okteto folder Jun 2, 2022

pchico83 approved these changes Jun 2, 2022

View reviewed changes

ifbyol approved these changes Jun 2, 2022

View reviewed changes

jLopezbarb approved these changes Jun 6, 2022

View reviewed changes

teresaromero requested review from ifbyol and jLopezbarb June 7, 2022 09:20

ifbyol reviewed Jun 8, 2022

View reviewed changes

teresaromero added 3 commits June 9, 2022 09:16

refactor: added exception for .okteto folder at stack name

6d8fcb6

Signed-off-by: Teresa Romero <teresa@okteto.com>

refactor: change components stack name

17f9a7e

Signed-off-by: Teresa Romero <teresa@okteto.com>

refactor: destroy and deploy deployment only when old is okteto

ca58381

Signed-off-by: Teresa Romero <teresa@okteto.com>

teresaromero force-pushed the tere/2661-fix-name-stack branch from 4e07bf9 to ca58381 Compare June 9, 2022 07:16

ifbyol reviewed Jun 9, 2022

View reviewed changes

teresaromero added 2 commits June 9, 2022 10:03

refactor: fix deployed by label

89feccc

Signed-off-by: Teresa Romero <teresa@okteto.com>

docs: added comment for context over the check of the old stack label

7025b3a

Signed-off-by: Teresa Romero <teresa@okteto.com>

refactor: added test and fixes

fd04e04

Signed-off-by: Teresa Romero <teresa@okteto.com>

fix: spelling

0e65641

Signed-off-by: Teresa Romero <teresa@okteto.com>

teresaromero added release/breaking-change release/bug-fix labels Jun 10, 2022

teresaromero removed the release/breaking-change label Jun 10, 2022

jLopezbarb approved these changes Jun 10, 2022

View reviewed changes

teresaromero merged commit fe90604 into master Jun 10, 2022

teresaromero deleted the tere/2661-fix-name-stack branch June 10, 2022 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix stack name when manifest at .okteto folder #2742

Fix stack name when manifest at .okteto folder #2742

teresaromero commented Jun 1, 2022 •

edited

codecov bot commented Jun 1, 2022 •

edited

jmacelroy Jun 2, 2022

teresaromero Jun 2, 2022

ifbyol commented Jun 2, 2022 •

edited

teresaromero commented Jun 3, 2022

ifbyol commented Jun 3, 2022

teresaromero commented Jun 3, 2022

pchico83 commented Jun 3, 2022

teresaromero commented Jun 3, 2022

teresaromero commented Jun 6, 2022 •

edited

pchico83 commented Jun 6, 2022

teresaromero commented Jun 7, 2022

teresaromero commented Jun 7, 2022

ifbyol Jun 8, 2022

teresaromero Jun 8, 2022

teresaromero Jun 9, 2022

ifbyol Jun 9, 2022 •

edited

teresaromero Jun 9, 2022

ifbyol Jun 9, 2022

jLopezbarb commented Jun 9, 2022

teresaromero commented Jun 10, 2022 •

edited

Fix stack name when manifest at .okteto folder #2742

Fix stack name when manifest at .okteto folder #2742

Conversation

teresaromero commented Jun 1, 2022 • edited

Proposed changes

How to reproduce

codecov bot commented Jun 1, 2022 • edited

Codecov Report

jmacelroy Jun 2, 2022

Choose a reason for hiding this comment

teresaromero Jun 2, 2022

Choose a reason for hiding this comment

ifbyol commented Jun 2, 2022 • edited

teresaromero commented Jun 3, 2022

ifbyol commented Jun 3, 2022

teresaromero commented Jun 3, 2022

pchico83 commented Jun 3, 2022

teresaromero commented Jun 3, 2022

teresaromero commented Jun 6, 2022 • edited

pchico83 commented Jun 6, 2022

teresaromero commented Jun 7, 2022

teresaromero commented Jun 7, 2022

ifbyol Jun 8, 2022

Choose a reason for hiding this comment

teresaromero Jun 8, 2022

Choose a reason for hiding this comment

teresaromero Jun 9, 2022

Choose a reason for hiding this comment

ifbyol Jun 9, 2022 • edited

Choose a reason for hiding this comment

teresaromero Jun 9, 2022

Choose a reason for hiding this comment

ifbyol Jun 9, 2022

Choose a reason for hiding this comment

jLopezbarb commented Jun 9, 2022

teresaromero commented Jun 10, 2022 • edited

teresaromero commented Jun 1, 2022 •

edited

codecov bot commented Jun 1, 2022 •

edited

ifbyol commented Jun 2, 2022 •

edited

teresaromero commented Jun 6, 2022 •

edited

ifbyol Jun 9, 2022 •

edited

teresaromero commented Jun 10, 2022 •

edited