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

Trying to deflake CheckFilterTest #522

Merged
merged 2 commits into from
Mar 14, 2024
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Mar 12, 2024

https://github.com/jenkinsci/support-core-plugin/runs/22561627825 is not reproducibe locally but has also been observed in PCT (@cloudbees reference BEE-10081). The failure does not make much sense because AsyncResultCache.get should always succeed on the built-in node (Jenkins). Locally:

FINER	c.c.j.support.AsyncResultCache#get: com.cloudbees.jenkins.support.impl.NetworkInterfaces$GetNetworkInterfaces@6ba48c4d on master succeeded
FINER	c.c.j.support.AsyncResultCache#get: com.cloudbees.jenkins.support.impl.NetworkInterfaces$GetNetworkInterfaces@35c553e2 on agent0 succeeded

@jglick jglick requested a review from a team as a code owner March 12, 2024 15:40
@@ -434,17 +430,4 @@ private boolean match(String s) {
return Pattern.matches(pattern, s);
}
}

private static class SimpleBuilder extends Builder implements SimpleBuildStep, Serializable {
Copy link
Member Author

Choose a reason for hiding this comment

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

Introduced in #174 but never used.

@@ -439,7 +439,7 @@ private String getInfo(ContentFilter filter) {
.append(" arg[")
.append(count++)
.append("]: `")
.append(Markdown.escapeBacktick(ContentFilter.filter(filter, arg)))
.append(Markdown.escapeBacktick(filter != null ? ContentFilter.filter(filter, arg) : arg))
Copy link
Member Author

Choose a reason for hiding this comment

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

Turning on fine logging revealed

FINE	c.c.j.support.AsyncResultCache#get: Could not retrieve Java info from agent0
Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to agent0
		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1784)
		at hudson.remoting.UserRequest$ExceptionResponse.retrieve(UserRequest.java:356)
		at hudson.remoting.Channel$2.adapt(Channel.java:1034)
		at hudson.remoting.Channel$2.adapt(Channel.java:1030)
		at hudson.remoting.FutureAdapter.get(FutureAdapter.java:66)
		at com.cloudbees.jenkins.support.AsyncResultCache.get(AsyncResultCache.java:69)
		at com.cloudbees.jenkins.support.impl.AboutJenkins$NodesContent.printTo(AboutJenkins.java:808)
		at com.cloudbees.jenkins.support.api.PrefilteredPrintedContent.writeTo(PrefilteredPrintedContent.java:63)
		at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:420)
		at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:355)
		at com.cloudbees.jenkins.support.CheckFilterTest.assertComponent(CheckFilterTest.java:158)
		at com.cloudbees.jenkins.support.CheckFilterTest.checkFilterTest(CheckFilterTest.java:92)
		at …
java.lang.NullPointerException
	at com.cloudbees.jenkins.support.filter.ContentFilter.filter(ContentFilter.java:109)
	at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.getInfo(AboutJenkins.java:442)
	at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.call(AboutJenkins.java:227)
	at com.cloudbees.jenkins.support.impl.AboutJenkins$GetJavaInfo.call(AboutJenkins.java:214)
	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
	at …
Caused: java.util.concurrent.ExecutionException
	at hudson.remoting.Channel$2.adapt(Channel.java:1036)
	at hudson.remoting.Channel$2.adapt(Channel.java:1030)
	at hudson.remoting.FutureAdapter.get(FutureAdapter.java:66)
	at com.cloudbees.jenkins.support.AsyncResultCache.get(AsyncResultCache.java:69)
	at com.cloudbees.jenkins.support.impl.AboutJenkins$NodesContent.printTo(AboutJenkins.java:808)
	at com.cloudbees.jenkins.support.api.PrefilteredPrintedContent.writeTo(PrefilteredPrintedContent.java:63)
	at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:420)
	at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:355)
	at com.cloudbees.jenkins.support.CheckFilterTest.assertComponent(CheckFilterTest.java:158)
	at com.cloudbees.jenkins.support.CheckFilterTest.checkFilterTest(CheckFilterTest.java:92)
	at …

Copy link
Member

@jtnord jtnord Mar 12, 2024

Choose a reason for hiding this comment

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

so this somehow is a production bug?
The filter should be filtering (anonymizing) sensitive info - and this if this is run on a JNLP agent the args may (IIUC) contain the Jenkins URL (which should be anonymized). But as stated in the javadoc this will never occur as the filter will be null for this case...?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think

// We make sure the output is filtered, maybe some labels are going to be filtered, but
// to avoid that:
// TODO: we have to change the MasterToSlaveCallable (GetJavaInfo) call to return a Map of
// values (key: value) and filter all the values here.
out.print(ContentFilter.filter(filter, javaInfo));
(#176) does that? It looks like #502 introduced the NPE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might have always been broken... See #473 (comment).

Copy link
Member

@jtnord jtnord Mar 14, 2024

Choose a reason for hiding this comment

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

so for PII / security reasons we should (if the filter is null) not include the args at all (at least as a temporary fix to allow the deflaking of this until the code can be propertly fixed in a different ticket? (that would IIUC not loose any info from agents as that info would not be available today as the code would have have blown up?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

for PII / security reasons…

Again as in #522 (comment) I think this content is already filtered, just in a clumsy way.

Anyway I file this patch in a separate PR for discussion since we need to get this test flake fixed.

Copy link
Member

Choose a reason for hiding this comment

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

👍 I mistakenly thought this was a partial cause of the flake (if an agent was connected at the time the bundle ran)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, probably unrelated (the flake was about the master node, weirdly enough), just something I saw when I turned on FINE logging.

@jglick jglick mentioned this pull request Mar 14, 2024
@jglick jglick requested review from Dohbedoh and jtnord March 14, 2024 12:04
@jglick jglick merged commit 3a46b9b into jenkinsci:master Mar 14, 2024
16 checks passed
@jglick jglick deleted the CheckFilterTest branch March 14, 2024 13:12
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