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

Add a text to explain the time window for GC #18735

Merged
merged 1 commit into from
May 26, 2023

Conversation

AllForNothing
Copy link
Contributor

@AllForNothing AllForNothing commented May 25, 2023

1.Fixes #18692
UI change:
image

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(issue)

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@AllForNothing AllForNothing requested a review from a team as a code owner May 25, 2023 07:41
@AllForNothing AllForNothing added the release-note/enhancement Label to mark PR to be added under release notes as enhancement label May 25, 2023
@AllForNothing
Copy link
Contributor Author

AllForNothing commented May 25, 2023

@OrlinVasilev Can you help to review the English text in this PR?
We add this text to explain the time window for GC

@Vad1mo Vad1mo added the area/i18n Related to Translation changes. label May 25, 2023
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #18735 (db3421d) into main (06aa87a) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18735      +/-   ##
==========================================
- Coverage   67.39%   67.37%   -0.03%     
==========================================
  Files         980      980              
  Lines      106749   106749              
  Branches     2665     2665              
==========================================
- Hits        71946    71921      -25     
- Misses      30940    30959      +19     
- Partials     3863     3869       +6     
Flag Coverage Δ
unittests 67.37% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files with indirect coverage changes

Copy link
Member

@Vad1mo Vad1mo left a comment

Choose a reason for hiding this comment

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

IMO the text is vague. It has too much room for interpretation. Furthermore, there are words that actually need to have an introduction and explanation because the meaning is not clear or vague:

Specifically:

  • How long is the time window
  • What is happens inside and outside this time window?
  • What does "covers" mean exactly?

Isn't the time window is hard-coded to 2h?

Why not write it in more simple words, explaining the restriction?

We can turn this -> Any garbage collection operation (including the dry run) only covers images that were not uploaded within the specified time window"

In more in simple words, straight to the point, that doesn't need any additional lookup.

  • EN: Artifacts uploaded in the past 2 hours are excluded from garbage collection.
  • DE: Artefakte, die in den vergangenen 2 Stunden hochgeladen wurden, sind von der Garbage Collection ausgenommen.
  • ES: Los artefactos cargados en las últimas 2 horas se excluyen de la recogida de basura.**

@OrlinVasilev
Copy link
Member

Agree with @Vad1mo we need clear and simple explanations.
and based on the docs Vadim's proposal is clear and correct
@AllForNothing can you implement it this way please

@AllForNothing
Copy link
Contributor Author

@OrlinVasilev @Vad1mo Thanks for the comments.
But I don't know if '2 hrs' is proper here, because the time window is an environment variable that is hard-coded as 2 hrs by default, and users can modify it.
What about "Artifacts uploaded within the specified time window(the default is 2 hrs) are excluded from garbage collection"?

@OrlinVasilev
Copy link
Member

@AllForNothing if it's tuneable variable the documentations is not pointing that: https://goharbor.io/docs/2.8.0/administration/garbage-collection/#run-garbage-collection

To avoid damaging the uploading artifact, the garbage collection introduces a time windows(2 hours) to reserve the recent uploaded layers.

Can we read that setting and display the value that the user has setup ?

@AllForNothing
Copy link
Contributor Author

Can we read that setting and display the value that the user has setup ?

@OrlinVasilev No, users can not read it. because we don't want to expose this to the user, but the user can surely override this value in the environment variable:
I think the explanation in the documentation is very clear.
So would it be better to be consistent with the documentation here(say 'a time window(2 hours)')?

@OrlinVasilev
Copy link
Member

@AllForNothing can you point me to that part of the documentation where is listed that you can tune that variable ?
So the messaage can be :

Artifacts uploaded in the past 2 hours(the default window) are excluded from garbage collection.

@AllForNothing
Copy link
Contributor Author

@OrlinVasilev This is not included in our doc.
This should be a part of development instruction(Unfortunately, we don't have it)
Can modify it by below steps
sudo -E make -e down add GC_TIME_WINDOW_HOURS=2 to make/common/config/core/env sudo -E make -e start

@AllForNothing
Copy link
Contributor Author

@OrlinVasilev @Vad1mo I will update my PR later according to the text 'Artifacts uploaded in the past 2 hours(the default window) are excluded from garbage collection.'

@OrlinVasilev
Copy link
Member

@AllForNothing by simple search looks like there are not many other env variables that we use to tune some services.
In that case I think we have to mention that somewhere otherwise it's super well hidden, and it doesn't make sense to have it configurable if we are not providing the way to configure it :)

@wy65701436
Copy link
Contributor

wy65701436 commented May 25, 2023

@Vad1mo @OrlinVasilev The original intention of this tooltip is to inform users that the garbage collector will not remove deleted artifacts within the specified time window, which is set to 2 hours by default. This is done to prevent any potential damage to artifacts that are currently being uploaded. Many users do not read the official documentation and often inquire about this behavior.

Regarding the tuning aspect, it was not originally intended for users to customize. Instead, it was primarily designed for debugging purposes. In other words, if a user sets the tuning value to 0 (disabled), it may cause issues in concurrent scenarios where the GC could potentially disrupt the pushing of artifacts. This pushing process is a critical pathway within the registry. So, in any of production environment, we don't recommend user to reset it. If we would like to doc it, @AllForNothing please file another issue specifically addressing this matter. Let's keep the review focused on the wording of the tooltip.

@AllForNothing My suggestion is to inform the user about the existing 2-hour time window in the GC(required) and provide a brief explanation of its purpose(optional). However, for more detailed information on why we have implemented this time window, we recommend referring to the documentation.

1.Fixes goharbor#18692

Signed-off-by: AllForNothing <sshijun@vmware.com>
Copy link
Member

@OrlinVasilev OrlinVasilev left a comment

Choose a reason for hiding this comment

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

lgtm

@Vad1mo Vad1mo merged commit 11d6bb4 into goharbor:main May 26, 2023
WilfredAlmeida pushed a commit to WilfredAlmeida/harbor that referenced this pull request Jul 8, 2023
1.Fixes goharbor#18692

Signed-off-by: AllForNothing <sshijun@vmware.com>
Signed-off-by: Wilfred Almeida <60785452+WilfredAlmeida@users.noreply.github.com>
WilfredAlmeida pushed a commit to WilfredAlmeida/harbor that referenced this pull request Jul 8, 2023
1.Fixes goharbor#18692

Signed-off-by: AllForNothing <sshijun@vmware.com>
@Vad1mo Vad1mo added the area/gc label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gc area/i18n Related to Translation changes. release-note/enhancement Label to mark PR to be added under release notes as enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarifying the action of button "GC NOW"
5 participants