added 1 commit

• 7fc07891 - Add a cleaned environment name to environments

Compare with previous version

@ayufan I think we do need these environment variables, to ensure people can create an environment in kubernetes/openshift with the expected name. Making them work out the correct name is a bit mean. I'll add it tomorrow.

Also, I've left the ability to update clean_name out of the API for now. It would be trivial to add, is it worth having it in the API before putting it in the UX? Bearing in mind how easy it would be for users to shoot themselves in the foot with it.

/cc @markpundsack

marked the task Documentation created/updated as completed

added 1 commit

• 2f9fb55d - Add a cleaned environment name to environments

Compare with previous version

marked the task Changelog entry added as completed

marked as a Work In Progress

assigned to @nick.thomas

marked the task All builds are passing as completed

added 2 commits

• bc1e4915 - Create environments when the build referencing them is created
• fa63e555 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG

Compare with previous version

marked the task All builds are passing as incomplete

mentioned in merge request !7690 (merged)

@adamniedzielski you'll need this one too. tl;dr use the environment's clean_name for kuberentes/openshift labels.

assigned to @ayufan

@ayufan can you review?

unmarked as a Work In Progress

Hmm. If the first byte of the environment name is a letter, we can omit 'env-', and extend the $CI_ENVIRONMENT_NAME contribution from 13 to 17 bytes.

!env.clean_name.present? -> env.clean_name.blank? ?

self. is not necessary here.

How does it work? 😄 The SQL query length is dependent on the number of rows? Is there a limit of SQL query length? (MySQL and Postgres)

On staging we have around 7000 environments.

We load them all into memory, then iterate through them, issuing one UPDATE environments SET clean_name = ? WHERE id = ? statement per environment.

When there's a lot of rows, the right thing to do is add a loop, and a "LIMIT n" statement to the SELECT. I guess this is a little marginal

@yorickpeterse would you rather this was done in batches of a thousand or so?

Oh right, this is an UPDATE statement not INSERT - we don't construct one big query for all rows. You can ignore the part of my comment about query length.

An alternative way would be to use Rails find_each which has the advantage of working in batches and the disadvantage of instantiating Ruby objects.

We can't rely on the Environment model in a migration. It may have changed by the time the migration is run.

OK, I've run this migration (and a second I added for the index) against a local postgres packed with 10,000 environments. Here's the output:

== 20161207231626 AddCleanEnvironmentName: migrating ==========================
-- add_column(:environments, :clean_name, :string)
   -> 0.0008s
-- change_column_null(:environments, :clean_name, false)
   -> 0.0124s
== 20161207231626 AddCleanEnvironmentName: migrated (4.5368s) =================

== 20161209153400 AddUniqueIndexForCleanEnvironmentName: migrating ============
-- transaction_open?()
   -> 0.0000s
-- add_index(:environments, [:project_id, :clean_name], {:unique=>true, :algorithm=>:concurrently})
   -> 0.0243s
== 20161209153400 AddUniqueIndexForCleanEnvironmentName: migrated (0.0249s) ===

So 4 seconds and 10,001 queries to do the update, and 0.03 seconds to add the index. I've marked both migrations as requiring downtime to ensure that uniqueness is enforced, I don't see a way around that particularly.

@nick.thomas If all rows can be updated using the same values we should definitely do this using batches. Updating rows 1 by 1 is going to be really slow.

@yorickpeterse unfortunately they can't. We're calculating the value of clean_name based on the contents of name. I guess we could introduce an SQL function that does the calculation for us, but given the timings we're seeing here, I don't think it's necessary.

There aren't many environments on GitLab.com, and I'd hazard there won't be that many environments elsewhere either. 10,000 (more than GitLab.com) in 4 seconds on an untuned postgres doesn't seem awful to me.

What do you think about replacing (0..5).map { SUFFIX_CHARS.sample }.join("") with SUFFIX_CHARS.sample(5).join("")?

sample(n) won't return the same character twice, so that would reduce entropy. I think I'm going to split this out into a random_suffix method though.

@nick.thomas Thanks for this! 💯

Why do we need to add the random suffix? Is it because after taking only first 12 characters the name may be no longer unique? If it's the case, can we better document it? (because it won't be so obvious for a person reading the code after 3 months)

What do you think about adding the unique constraint for clean_name in the database? Rails unique validation is just a polite suggestion that something should be unique, but it doesn't really give you a guarantee. I know that we don't have unique constraint in the database for name 😞.

The random suffix is there because it would be really easy for two cleaned and shortened environment names to conflict. We are lacking docs on what the cleaned name looks like, I can add that to doc/.../environments and an inline comment.

A unique index on [project_id, clean_name] is definitely worthwhile, it slipped my mind!

Oh, in addition to not bothering with the prefix if we start with a letter, we can also not bother with the suffix if cleaned == name

Some examples:

• review/foo => review-foo-5gghdf
• review-foo => review-foo
• 1-foo => env-1-foo-e2hx12
• production => production
• Production => production-f8ddlz

Wdyt? It's that production one that I think is the part that makes the extra complexity of the rules worthwhile. As long as we're putting the cleaned name into the runner environment / making it available to smart deploy, making it more complicated to calculate shouldn't be an issue. Indeed, including the random suffix means it can't be calculated

added 2 commits

• bcc692b4 - fixup! Add a cleaned environment name to environments
• d582a474 - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG

Compare with previous version

It should be all-lowercase too. So Production -> production, etc, and no uppercase in the suffix either.

added 1 commit

• f472a7df - fixup! Add a cleaned environment name to environments

Compare with previous version

added 87 commits

• f472a7df...e17758c2 - 84 commits from branch gitlab-org:master
• cafae647 - Add a cleaned environment name to environments
• 49f9aaa2 - Create environments when the build referencing them is created
• fa57cbdc - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG

Compare with previous version

@adamniedzielski just FYI there is a unique index on [project_id, name] as well.

Hmm, I think this should probably be $CI_ENVIRONMENT_SLUG if the other one is going to be called $CI_BUILD_REF_SLUG

OK, I've pushed this change as a fixup. We can rebase before merge to include it if you agree.

added 1 commit

• f9d8cfe5 - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG

Compare with previous version

mentioned in issue #22849 (closed)

Build failure will be fixed by !8014 (merged), will rebase then

added 31 commits

• f9d8cfe5...3a66c239 - 26 commits from branch gitlab-org:master
• 81a4a01b - Add a cleaned environment name to environments
• 870da923 - Create environments when the build referencing them is created
• 7be4f593 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG
• 2251c3ca - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG
• 220b37ff - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_NAME_SLUG

Compare with previous version

project_id is obsolete identifier which is to be removed in 9.0. Use gl_project_id for now.

Maybe name it name_slug or just slug?

slug it is 👍

Should we mnemonize this call?

Hmm. It wasn't before, and this call is no more expensive now than it was.

Since it's only called in variables, I'd suggest memoising that method, rather than this one, if any.

Ah no, it's called in expanded_environment_name too.

Benchmarking it, this call takes <1ms. I'm not worried by that.

Should we mnemonize this call?

I'll benchmark it ^{^}

Once the database objects (including @persisted_environment) are loaded, this one is also sub-millisecond, so I'm not worried here either.

Rampant memoisation carries a complexity cost - you have to remember to throw away the instance when certain things change. It's justifiable for persistent_environment, but I don't think it's justifiable here. Wdyt?

You guys are killing me

I'm fine with leaving that as is. You are right it have minimal impact now. I think that we should aim to have a Service to construct all variables, because as of right now all this logic happens in models which is not ideal, but this is improvement for a next iteration :)

Oh. NIce.

Can you check if this call does update URL of environment?

I purposefully left the URL out here, for two reasons:

1. It is counter-intuitive to have updated the URL if the deployment fails
2. I'm not exposing the URL as a variable, so don't need it.

Sure, can you check if it gets updated later? I remember seeing an issue with this.

But I also consider that we should add: CI_ENVIRONMENT_URL to exposed variables, because it makes a life a hell a lot easier when creating deployment.

Right now we will have to do:

deploy:
  script:
    - create-route https://$CI_ENVIRONMENT_SLUG.example.com
 environment:
   url:  https://$CI_ENVIRONMENT_SLUG.example.com

We could generate a CI_ENVIRONMENT_URL from data stored in Ci::Build, but it kind of generates a mess. Not sure what is the best approach here.

I would prefer the CI_ENVIRONMENT_URL to be a separate issue and merge request to not prevent us from merging this one.

Issue: #25680 (closed)

The environment URL gets updated in CreateDeploymentService, which is called by BuildSuccessWorker. There's a spec for "environment URL is updated when the build succeeds if the environment already exists":

CreateDeploymentService
  #execute
    when variables are used
      and environment exist
        updates external url

i've also tested it manually and observed that a .gitlab-ci.yml like this:

environment:
  name: pipeline-$CI_PIPELINE_ID
  url: http://$CI_ENVIRONMENT_SLUG.example.com

results in an environment like this:

=> #<Environment:0x007fc109e508b8
 id: 10003,
 project_id: 10,
 name: "pipeline-76",
 created_at: Wed, 14 Dec 2016 14:11:30 UTC +00:00,
 updated_at: Wed, 14 Dec 2016 14:15:32 UTC +00:00,
 external_url: "http://pipeline-76.example.com",
 environment_type: nil,
 state: "available",
 slug: "pipeline-76">
[2] pry(main)>

I can't find an issue referencing environment.external_url not getting updated, although there are several reporting problems with variable expansion (e.g., #24256 (closed) and #22970 (closed) - is that what you're referring to?

Oh. Right. I thought that this is broken, but it seems to be completely fine :) Sorry.

I'm thinking that we should store the environment_id as part of the build.

Not sure if we should make this part of this MR.

The presence of the environment string column complicates this somewhat. We'd need to rename it, or replace all uses of it with read_attribute or write_attribute. I agree that it would be nicer than this.

It's struck me that we have ten days left 😱

Separate issue, technical debt label?

Separate issue and technical debt. This is something that we should improve, as inter-dependencies between builds, deployments and environments become kind of complicate. We can also add a comment to this one: #24723

Issue: #25684

The performance characteristics of this as-is are reasonable-ish, as we're using the index on [project_id, name] to find the environment and memoising it, but I reckon we should schedule this refactoring for 8.16. We can then take advantage of eager loading, etc.

Exactly. Thanks for creating an issue :)

Should we move this method to lib/?

I don't think we should. It doesn't have broad applicability - $CI_BUILD_REF_SLUG will have different rules - and we shouldn't call it from a database migration, either.

Should we also pass CI_ENVIRONMENT_URL?

If we do, then it should not come from here. Related: !7983 (comment 19925550)

If an environment exists, and I push an updated .gitlab-ci.yml that changes the environment URL, it would be strange for the URL to update before that build succeeds. The name and slug don't have that issue - if they change, a new environment is created.

We can publish $CI_ENVIRONMENT_URL, but if we do, it should be added in app/models/ci/build.rb, rather than here, I think.

OK, I think this should go into a separate issue if we want it. Adding it isn't trivial - the build doesn't have the un-expanded environment url in the same way it has the environment name - and I think it will be used much less often inside the runner than the environment slug will be. We need the latter to set up kubernetes & openshift environments, etc, but I've not come across a need to have the URL yet.

Should we return 403 if the change is forbidden?

The general approach in the API at the moment is to filter out paramers that aren't recognised as valid for update. PUT ... {"id": 666} won't result in a 403 either. I think it makes more sense to be consistent here.

Not sure. I would expect to receive 400 if something that I'm changing is forbidden.

Hmm, not sure:

• If we do receive in API entity the slug I would expect that I can play with that,
• By making 200 we silently ignore the change, making the API to behave strangely,

Throwing an idea, maybe we should add to put endpoint:

requires :slug, presence: false

@ayufan If we just update the PUT endpoint, then it is inconsistent with the POST endpoint, which will silently ignore the parameter if present.

It will also still be inconsistent with the id parameter, which will be silently ignored on both PUT and POST.

It will also be inconsistent with the rest of the API. Everywhere else, the API silently strips out and ignores parameters that cannot be updated, and returns a 200 response.

I'd prefer 400 errors as well, but consistency is valuable and we can't make such a change across the whole API until 9.0 (I'll create an issue for this).

Since this is a new attribute, we have some flexibility to be inconsistent with the rest of the API, especially if we're declaring this a model for future direction . I think I'd be OK with any of these options:

• Allow slug to be set via POST or PUT, giving the user the power to shoot themselves in the foot
• Allow slug to be set via POST, return 400 on PUT
• Return 400 if slug is included in POST or PUT requests

Issue: #25688 (closed) - let me know how you want me to proceed here

I'm one of these guys that prefers to open, rather to make it to wide open.

I would prefer to return 400 if you want to set slug in POST or PUT.

requires :slug, presence: false doesn't have the effect we want. The require DSL method unconditionally introduces a PresenceValidator, and it only validates that the parameter is there, there isn't a code path for validating its absence.

I've added an Absence validator and used that, which does the trick.

@nick

Looks nice. A bunch of comments to you :)

assigned to @nick.thomas

Poor @nick :D. It's @nick.thomas

@adamniedzielski ah, no, you're right. there's an index, but it's not a unique index. Rubbish.

added 16 commits

• 220b37ff...db9e1635 - 13 commits from branch gitlab-org:master
• ef57d7d4 - Add an environment slug
• d5600fb6 - Create environments when the build referencing them is created
• ee8da488 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

added 16 commits

• 220b37ff...db9e1635 - 13 commits from branch gitlab-org:master
• ef57d7d4 - Add an environment slug
• d5600fb6 - Create environments when the build referencing them is created
• ee8da488 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

changed title from Add a cleaned name to environments to Add a slug to environments

added 1 commit

• c14a0930 - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

marked the task All builds are passing as completed

added 1 commit

• b06f42cb - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

mentioned in merge request !8072 (merged)

added 96 commits

• b06f42cb...7b63587d - 91 commits from branch gitlab-org:master
• df685833 - Add an environment slug
• e0281b27 - Create environments when the build referencing them is created
• 8ad59b5b - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG
• 9043a984 - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG
• e68e3265 - fixup! Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

assigned to @ayufan

@ayufan ready for second review, I think. I've pushed back against quite a few of the questions, but I think reasonably so! Happy to go into more detail if necessary.

added 1 commit

• 84133927 - fixup! Add an environment slug

Compare with previous version

I believe that - should not be allowed as last characted of environment slug :)

Good catch! This is a limitation of kubernetes labels.

I've implemented this so that "foo-" becomes foo-abcdef instead of foo--abcdef, which seems a bit nicer.

assigned to @nick.thomas

@nick.thomas

Good work. Only a few minor comments :)

mentioned in issue #25680 (closed)

mentioned in issue #25684

mentioned in issue #25688 (closed)

added 1 commit

• 358f79db - fixup! Add an environment slug

Compare with previous version

@ayufan great, let me know how to proceed with the API, and I'll implement that and rebase ready for (hopefully) final review :D

added 1 commit

• 4e85fbfd - fixup! Add an environment slug

Compare with previous version

resolved all discussions

added 80 commits

• 4e85fbfd...6910bf29 - 77 commits from branch gitlab-org:master
• b7477807 - Add an environment slug
• 75b2ae1a - Create environments when the build referencing them is created
• 34e07c07 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

@ayufan OK, the API now returns 400 in those cases. I've rebased and squashed.

No explicit tests of the Absence validator, because instantiating one turns out to be an incredible amount of work, but it's tested through its effects on the environments endpoint so I'm confident that it's right. It's mostly a straight copy of the PresenceValidator anyway!

assigned to @ayufan

Can this be a post deployment migration?

No, we have to ensure that no new environments are created while we're working.

Downtime for this is about 4 seconds with the data on staging.gitlab.com

Adding concurrent indexes doesn't require downtime.

In this case, we're adding a unique index. It's a long shot, but there's a chance that doing this online could result in a duplicate entry being created, causing this migration to fail.

Downtime for this is under a second, and since we're already down due to AddEnvironmentSlug, I thought the correctness was cheap enough.

Actually, it's not that long a shot. If we ran both migrations with DOWNTIME=false, with environments being created by runners connecting to old code versions, we'd end up with multiple rows with NULL as the value.

@nick.thomas final questions about migrations :)

@ayufan replied. More discussion on the database migrations can be found here: !7983 (comment 19753114)

@nick.thomas Fair enough. Did you have timings for these migrations from staging? Just to be 100% sure.

added 5 commits

• 34e07c07...2f736c6d - 2 commits from branch gitlab-org:master
• aff3cf39 - Add an environment slug
• 481e77b1 - Create environments when the build referencing them is created
• 7911266b - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

@ayufan the numbers mentioned above were run on my local machine.

I just re-ran them on staging, inside a transaction. 9 seconds (rather than 4) for the first migration.

The second migration actually fails, because a duplicate column exists in the staging data. Am I going to need write a migration to deal with this case and convert the index on environment name to a unique index as part of this MR?

Oh, and I fixed the down call in the first migration, as it still incorrectly referenced clean_name rather than slug !!!

There are two duplicate rows on staging. I'll write a migration to clean up environments.name so this can't happen again

added 4 commits

• c05591d5 - Make the index on environment name and project id unique, fixing up any duplicates
• 66007706 - Add an environment slug
• b7f47789 - Create environments when the build referencing them is created
• 10e4ddc0 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

added 8 commits

• 10e4ddc0...79f74249 - 4 commits from branch gitlab-org:master
• acaa7111 - Make the index on environment name and project id unique, fixing up any duplicates
• 860fb4de - Add an environment slug
• 548677b0 - Create environments when the build referencing them is created
• 1fd6cdd7 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

Sorry @ayufan, this is some technical debt that couldn't be punted to a separate issue :/

So, on staging.gitlab.com, the first two migrations - fix up non-unique environment names and swap the non-unique index for a unique index - both complete in under a second.

We only operate on rows that aren't unique (HAVING is the best SQL feature ever).

The third migration, generating slugs for all 7000 rows on staging, takes just under 10 seconds.

The fourth migration duplicates the add_concurent_index call of the second, but for slugs rather than names, and also takes under a second.

So it looks bad, but in reality it's OK.

added 13 commits

• 1fd6cdd7...4e68b881 - 9 commits from branch gitlab-org:master
• d12f91b2 - Make the index on environment name and project id unique, fixing up any duplicates
• 92276471 - Add an environment slug
• e00e25dd - Create environments when the build referencing them is created
• e1f87011 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

added 4 commits

• 27ae7cae - Make the index on environment name and project id unique, fixing up any duplicates
• fa71d422 - Add an environment slug
• 890506b1 - Create environments when the build referencing them is created
• 09e5e97e - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

@nick.thomas Can we ask @yorickpeterse what he thinks about it?

mentioned in commit 51830b66

@nick.thomas I have a hard time reading these queries, so judging them is tricky. How does this perform when there are for example 10 000 environments (or something along those lines) that are affected? If I'm not mistaken this will run 1 UPDATE per row, which could be very slow. Is it perhaps possible to update rows using a CASE, something like:

UPDATE environments
SET whatever = 
  CASE
  WHEN foo = bar THEN 1
  WHEN foo = baz THEN 2
  END
;

This should at least reduce the number of queries.

@yorickpeterse there's a uniqueness constraint in theEnvironment model, so I don't expect large numbers of affected environments anywhere. We touch four rows on staging, for instance.The queries we run for this migration are:

SELECT project_id, name FROM environments GROUP BY project_id, name HAVING count(1) > 1

This returns 2 rows on staging.gitlab.com

For each row:

SELECT id,name FROM environments WHERE project_id = $1 AND name = $2

For staging, this is run twice and returns 2 rows each time.

For the second row of each of those sets:

UPDATE environments SET name = $1 WHERE id = $2

So this is a total of 5 queries, and 2 UPDATE statements, inside a transaction. I don't think we need the CASE enhancement - we're not using it 2 migrations later, which is one update per row.

@nick.thomas Ah, that clarifies things. I tested the queries for GitLab.com (production) and also only 2 rows are returned. As such I think these migrations should be fine as I don't expect anybody to have more than a handful of rows. 👍

Does this require downtime because it depends on application code not generating duplicates?

Right. There's a chance that old code could be running and attempting to create new environments at the time, so races undoing all the good work of the fixup migration could happen.

The ('0'..'9') range can be moved to a constant so we don't end up creating a Range for every row.

👍 I'll add that to the migration and the model

mentioned in issue #23580 (closed)

added 19 commits

• 09e5e97e...2d1dfae9 - 15 commits from branch gitlab-org:master
• 35a3e918 - Make the index on environment name and project id unique, fixing up any duplicates
• 93a03cd9 - Add an environment slug
• 58486918 - Create environments when the build referencing them is created
• 80513a12 - Add $CI_ENVIRONMENT_NAME and $CI_ENVIRONMENT_SLUG

Compare with previous version

@ayufan ok, comments from @yorickpeterse addressed I believe. No major changes to the migrations were requested. Another review?

resolved all discussions

merged

mentioned in commit dfdce39a

mentioned in merge request gitlab-com/www-gitlab-com!4007 (merged)