Skip to content

Eric/use mule in jenkins#1072

Merged
algojohnlee merged 88 commits intoalgorand:masterfrom
egieseke:eric/useMuleInJenkins
Jun 22, 2020
Merged

Eric/use mule in jenkins#1072
algojohnlee merged 88 commits intoalgorand:masterfrom
egieseke:eric/useMuleInJenkins

Conversation

@egieseke
Copy link
Copy Markdown
Contributor

Summary

Create a Jenkins mule pipeline to build and test go-algorand.
https://github.com/algorand/go-algorand-internal/issues/424

Test Plan

Setup and executed Jenkins pipeline:

https://jenkins.algodev.network/job/JenkinsMulePipeline/

bricerisingalgorand and others added 30 commits May 1, 2020 12:43
This includes changes that allow travis to use mule to execute our ci process
Multiple proxies are not supported in go 1.12, so since proxies fail occasionally, we'll use only direct for now. After upgrading to 1.13, we'll add proxies here
These are failing inconsistently with timeouts. Increasing it since it seems to be passing most of the time
Comment thread test/scripts/e2e_basic_start_stop.sh Outdated
echo Verifying a generic node will start directly
${BINDIR}/algod -d ${DATADIR} &
algod -d ${DATADIR} &
sleep 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this does becomes an issue, this is not the correct place to fix it. you might want to try to sleep in case the pgrep returns no pids in update_running_count

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm surprised the answer isn't to make verify_at_least_one_running have a retry loop like verify_one_running has

Copy link
Copy Markdown
Contributor Author

@egieseke egieseke Jun 12, 2020

Choose a reason for hiding this comment

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

@tsachiherman @brianolson I just updated verify_at_least_one_running to include the retry loop and removed the sleep statements. Good thinking.

@egieseke egieseke requested a review from tsachiherman June 13, 2020 14:29
proc ::AlgorandGoal::TakeAccountOnline { ADDRESS FIRST_ROUND LAST_ROUND TEST_PRIMARY_NODE_DIR } {
if { [ catch {
spawn goal account changeonlinestatus --address $ADDRESS --firstvalid $FIRST_ROUND --lastvalid $LAST_ROUND -d datadir
spawn goal account changeonlinestatus --address $ADDRESS --firstvalid $FIRST_ROUND --lastvalid $LAST_ROUND -d datadir --online
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this change is supposed to be a no-op. i.e. the default for --online is true. If this is not the case, then we have a bug, which is unrelated to this PR.

Copy link
Copy Markdown
Contributor Author

@egieseke egieseke Jun 14, 2020

Choose a reason for hiding this comment

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

@tsachiherman The --online is required by the goal account changeonlinestatus command. Without it, the command fails. I will create a ticket to fix the command and update this test case to remove the --online option.
https://github.com/algorand/go-algorand-internal/issues/541.
Created pull request: #1170

@tsachiherman Reverted the addition of the --online flag since it will no longer be necessary.

Copy link
Copy Markdown
Contributor

@tsachiherman tsachiherman Jun 15, 2020

Choose a reason for hiding this comment

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

The PR(#1170) looks good to me, but I wonder how this test did not detect this issue before.
It's quite concerning that we have a test that is supposed to test exactly one thing - and the test doesn't alert us when that thing is broken.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We found this issue by running the integration tests as part of the Jenkins/Mule pipeline testing. The issue was causing the test test/e2e-go/cli/goal/expect/listExpiredParticipationKeyTest.exp to fail. What is puzzling is why this error does not surface as part of the Travis build and testing. I created this ticket to resolve: https://github.com/algorand/go-algorand-internal/issues/542

Copy link
Copy Markdown
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

Hmm, the mule-specific changes here look okay at a glance, but the test changes give me pause. Is it possible to add commentary around changes as to why they were made?

Comment thread scripts/configure_dev.sh
@egieseke
Copy link
Copy Markdown
Contributor Author

Hmm, the mule-specific changes here look okay at a glance, but the test changes give me pause. Is it possible to add commentary around changes as to why they were made?

The resource-constrained platforms require more time to complete the steps of the tests. Especially for the expect tests that are running 2 node networks for goal to interact with.

Comment thread scripts/release/mule/Makefile.mule Outdated
mkdir -p $(SRCPATH)/tmp/node_pkgs && \
CHANNEL=$(BUILDCHANNEL) PKG_ROOT=$(SRCPATH)/tmp/node_pkgs NO_BUILD=True VARIATIONS=$(OS_TYPE)-$(ARCH) \
scripts/build_packages.sh $(OS_TYPE)/$(ARCH) && \
mkdir -p $(SRCPATH)/tmp/node_pkgs/$(BUILDCHANNEL)/$(OS_TYPE)-$(ARCH)/data && \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm pretty sure that this is going to break the existing mule tasks (see the package*.yaml files in the root dir) because this path is different.

If you look at the PKG_ROOT var in the deleted snippet above it's expecting the build assets to be put in a different path than the one you have now.

Copy link
Copy Markdown
Contributor Author

@egieseke egieseke Jun 17, 2020

Choose a reason for hiding this comment

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

@btoll Please see updates.

Comment thread scripts/release/mule/Makefile.mule Outdated
export PATH=$(SRCPATH)/tmp/node_pkgs/$(BUILDCHANNEL)/$(OS_TYPE)-$(ARCH)/bin:$$PATH && \
export PATH=$(SRCPATH)/tmp/node_pkgs/$(BUILDCHANNEL)/$(OS_TYPE)-$(ARCH)/tools:$$PATH && \
export PATH=$(SRCPATH)/tmp/node_pkgs/$(BUILDCHANNEL)/$(OS_TYPE)-$(ARCH)/test-utils:$$PATH && \
export SRCROOT=$(SRCPATH) && \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like these env vars should be precede the call to the shell script rather than exporting them before the call to the script.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Look below to see where the call to test/scripts/e2e.sh is appended to the series of exports.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The script needs the env vars, not the calling environment. It's a common shell idiom to do:

FOO=foo BAR=bar my_script.sh

Also, the ci-build target is doing it that way and it would be great to keep that consistency.

I realize this was existing code, but it would be great if you could change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@btoll please see updates.

Comment thread scripts/release/mule/Makefile.mule
@btoll
Copy link
Copy Markdown

btoll commented Jun 17, 2020

@egieseke Thanks for the changes, I added one more comment.

@btoll
Copy link
Copy Markdown

btoll commented Jun 17, 2020

Just approved. I'm assuming the brittleness of the tests will be addressed in future tickets.

@egieseke egieseke requested a review from onetechnical June 18, 2020 16:24
Copy link
Copy Markdown
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

I'm okay with this, but let's pay close attention to how it affects tests. This is also predicated on us doing a deeper dive into tests.

@algojohnlee algojohnlee merged commit a87718f into algorand:master Jun 22, 2020
cce added a commit to cce/go-algorand that referenced this pull request Jul 30, 2025
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

Successfully merging this pull request may close these issues.

8 participants