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 cmdLineTester_criu_keepCheckpoint test #15516

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

LongyuZhang
Copy link
Contributor

@LongyuZhang LongyuZhang commented Jul 11, 2022

Signed-off-by: Longyu Zhang longyu.zhang@ibm.com

@LongyuZhang
Copy link
Contributor Author

@llxia @tajila Could you review it? Thanks.

@LongyuZhang
Copy link
Contributor Author

Test link: hyc_grinder 25600

then
sleep 2;
criu restore -D ./cpData --shell-job;
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having if for SingleCheckpoint, TwoCheckpoints, and ThreeCheckpoints, should we have a loop?

Copy link
Contributor Author

@LongyuZhang LongyuZhang Jul 12, 2022

Choose a reason for hiding this comment

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

A loop has been added here. New grinder link is 25621. Thanks.

<variable name="MAINCLASS_SIMPLE" value="org.openj9.criu.CRIUSimpleTest" />

<test id="Create Criu Checkpoint Image">
<command>bash $SCRIPPATH$ $TEST_RESROOT$ $JAVA_COMMAND$ $JVM_OPTIONS$ $MAINCLASS_SIMPLE$ SingleCheckpoint OnlyCheckpoint</command>
Copy link
Contributor

Choose a reason for hiding this comment

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

  • This looks like it is duplicating
    <command>bash $SCRIPPATH$ $TEST_RESROOT$ $JAVA_COMMAND$ $JVM_OPTIONS$ $MAINCLASS_SIMPLE$ SingleCheckpoint</command>
    with OnlyCheckpoint. Is it possible to add it to criu.xml?
  • I would prefer to have a boolean for $6, not the string OnlyCheckpoint. True for keeping the test output and false for deleting testOutput.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • $6 has been updated to a boolean value as suggested.
  • This test differs in output condition check from the referred one, e.g. "Post-checkpoint" output is considered as failure here since it should not restore checkpoint; while it is success for normal criu test. Also, if we merge these two xmls, since OnlyCheckpoint test and portable criu test only requires this single subtest, would we have ways to only run this subtest in playlist? Thanks.

do
sleep 2;
criu restore -D ./cpData --shell-job;
done
Copy link
Contributor

@llxia llxia Jul 12, 2022

Choose a reason for hiding this comment

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

I meant to set the checkpoint number directly (not a string), so we can set any number in the future.

NUM_CHECKPOINT=$5
for ((i=0; i<$NUM_CHECKPOINT; i++)); do
        sleep 2
        criu restore -D ./cpData --shell-job
done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR has been updated to take numbers of checkpoints as input, the grinder is hyc 25639. Thanks.

@LongyuZhang LongyuZhang changed the title Add cmdLineTester_criu_onlyCheckpoint test Add cmdLineTester_criu_keepCheckpoint test Jul 12, 2022
- Add cmdLineTester_criu_keepCheckpoint test
- This test is needed for portable criu docker test later

Signed-off-by: Longyu Zhang <longyu.zhang@ibm.com>
@llxia
Copy link
Contributor

llxia commented Jul 12, 2022

Could you provide the Grinder link? Thanks

@LongyuZhang
Copy link
Contributor Author

The new grinder link is hyc 25639. Thanks.

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

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

Thanks @LongyuZhang

@llxia llxia merged commit 4ac8e6a into eclipse-openj9:master Jul 13, 2022
# $3 is the JVM_OPTIONS
# $4 is the MAINCLASS
# $5 is the NUM_CHECKPOINT
# $6 is the KEEP_CHECKPOINT
Copy link
Contributor

Choose a reason for hiding this comment

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

@LongyuZhang is this needed? we have checkPointJVM(CRIUSupport criu, Path path, boolean deleteDir)

Copy link
Contributor Author

@LongyuZhang LongyuZhang Jul 13, 2022

Choose a reason for hiding this comment

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

The parameter KEEP_CHECKPOINT is currently used as the condition for both restore control and testOutput file; the deleteDir is more about the folder and files, but maybe not for whether restore or not? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants