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

various pipeline improvements #1454

Merged
merged 34 commits into from
Sep 28, 2023

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Sep 26, 2023

No description provided.

@@ -86,6 +86,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<version>3.1.2</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is an interesting pattern that I have observed with all builds that have timed out recently for a few days and we kept re-triggering. All of them where hanging in one of the phases of maven-failsafe-plugin plugin, and this is why our builds were failing. It was hanging for even 20 minutes at times. Why? and why now? No idea, I can't even trigger a thread dump to understand what is going on, neither can I replicate this locally.

Instead what I found is a few mentions about this on the web, and everyone seemed to magically get away from the problem by upgrading. So I did the same here. What I have observed across this successful builds is that the "hang" is not there anymore, but we could simply be lucky here.

Either way, upgrading would not hurt at all, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the plugin version is maintained by boot? What version are we currently using? Is the version different in main? If we change the version, can we change in the the k8s parent pom in the dependency management section so we are not changing it everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    1. yes, it is maintained by boot
    1. 2.22.2 is what we are currently using
    1. main branch uses the same 2.22.2
    1. excellent point about the pluginManagement, will fix this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still uneasy about this change. Making a change to a plugin version purely due to the build system feels wrong to me...its going to put more burden on us to keep it up to date and also we are diverging from the spring dependency management.

The best solution I could think of that might work is to override the version specifically when we are building on github actions only. At least it isolates it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thank you for the feedback

@@ -61,7 +61,7 @@ runs:
./mvnw -s .settings.xml \
-DtestsToRun=${TEST_ARG[@]} \
-e clean install \
-U -P sonar -nsu --batch-mode \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was weird looking here: both -U and -nsu which is the opposite of -U.

In general, our .m2 is as fresh as it can get at this point, so there is no need for -U, we have already updated snapshots before this

@@ -3,10 +3,11 @@ description: clean space
runs:
using: "composite"
steps:
- name: apt-update
- name: gcloud
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going nuts with github actions in this step. It was failing the whole day for me in these sequence of events:

  • "what is the version of gcloud?" 477.0.0
  • "delete gcloud" - it's not present.

How can it be not present if you just gave me the version above? :) May be github was upgrading something in the background and I was caught in the middle, I don't know.

I am leaving a small print statement for gcloud version for a few weeks and will monitor this one across builds and eventually get rid of it. It's just an informative statement, does nothing else.

@wind57 wind57 changed the title test various pipeline improvements Sep 26, 2023
@wind57 wind57 marked this pull request as ready for review September 26, 2023 12:20
@@ -29,9 +29,6 @@ jobs:
- name: checkout project
uses: actions/checkout@v2

- name: clean space
Copy link
Contributor Author

@wind57 wind57 Sep 26, 2023

Choose a reason for hiding this comment

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

we don't need this one here, we do need to clean space, but not here, there is no pressure on this stage. A 5 minutes improvement on the overall build time :)

@wind57
Copy link
Contributor Author

wind57 commented Sep 26, 2023

@ryanjbaxter ready to be looked at, left comments everywhere. thank you

- name: apt-update

### this is supposed to be simpler, but it's a work-around for:
### https://github.com/jlumbroso/free-disk-space/issues/14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so some users started reporting the same issue that I am seeing, that simply using jlumbroso/free-disk-space@main fails in most recent builds. It fails saying that gcloud is not present and can not be removed. I assume github started packaging this artifact in a different manner, not via apt, so this breaks the plugin we were using.

Until there is a real fix for this, this is a work-around. Basically in plain english it says: remove everything that we were removing via large-packages: true, except gcloud. This one is removed a bit differently"

@wind57 wind57 requested a review from ryanjbaxter September 26, 2023 20:38
@wind57 wind57 marked this pull request as draft September 27, 2023 10:47
@ryanjbaxter
Copy link
Contributor

Is there a reason why you marked this as a draft again?

@wind57
Copy link
Contributor Author

wind57 commented Sep 27, 2023

yes, there is a PR where I merged this one and it still fails, I want to see why. Will ping you as soon as I figure out what is going on.

btw, main build failure is going to be fixed by this PR too, but I guess you already know this.

@wind57 wind57 marked this pull request as ready for review September 27, 2023 16:07
@wind57
Copy link
Contributor Author

wind57 commented Sep 27, 2023

@ryanjbaxter actually no, we can merge this one and I can investigate what is going on there and potentially have another fix.

@ryanjbaxter ryanjbaxter merged commit c88c9fa into spring-cloud:3.0.x Sep 28, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants