Changing bufio.Scanner to bufio.Reader to support large message

[CerberusQc]

From the Issue #3343

As stated in the bufio Documentation, when using the bufio.Scanner, if the buffer exceed the max size of the token, the Scanner fail silently with an I/O error.

In the case of a container started with nerdctl, the main process using stdout/stderr hang forever when sending text bigger than 64kb (bufio.MaxScanTokenSize) without any newline character. (Discovered from a JSON string dumped in the logs). bufio documentation state to switch to a Reader instead when you do not have control over the size of the message.

Small test that is failing with the current master but working with this fix.

@apostasie recommend to check the other place in the code for the bufio.Scanner usage but I am not really a GO expert

[apostasie]

@CerberusQc you need to DCO your commit with git -S (and squash your commits here).

[apostasie]

@apostasie recommend to check the other place in the code for the bufio.Scanner usage but I am not really a GO expert

That does not have to happen right now. Maybe if you can open a new issue, in the line of: "follow-up to #3343: review uses of bufio.Scanner", so that someone can pick that up later?

[CerberusQc]

@CerberusQc you need to DCO your commit with git -S (and squash your commits here).

@apostasie I will need help with this one, I have created a GPG key, signed the last 4 commits, when I tried to sign the first commit, it created a merge, now I can't squash correctly. I have no idea of what I am doing at this point and I am really not familiar with the process, sorry about this

[apostasie]

@CerberusQc you need to DCO your commit with git -S (and squash your commits here).

@apostasie I will need help with this one, I have created a GPG key, signed the last 4 commits, when I tried to sign the first commit, it created a merge, now I can't squash correctly. I have no idea of what I am doing at this point and I am really not familiar with the process, sorry about this

No problem.

So, first, the DCO is unrelated to actual key signing.
What you MUST do for the DCO when commiting is git commit -s ...
That will add the DCO to your commit message (https://wiki.linuxfoundation.org/dco)
(apologies as I previously said -S - this one if for signing, which is nice, but not required).
If you want to do both (best), git commit -S -s -m "..."

About the rest - tis not you, tis git :D

You have 5 commits here, just soft reset them (commits are rolled-back, but your changes are preserved locally):

git reset --soft HEAD~5

And now you can start "fresh": just add then commit

git add pkg/logging/logging.go pkg/logging/logging_test.go

# Some basic sanity checks before committing
make lint
make lint-imports

# Change to whatever message suits you
git commit -S -s -m "fix log issue with large messages (bufio.Scanner > bufio.Reader)"

# Now force-push
git push -f

Future stuff - if you do need more modifications, just amend instead of adding new commits on top:

git add whatever
git commit --amend
git push -f

Does that help?

Addenda:

My personal religion forbids me from touching GPG keys without a hazmat suit. If you feel similarly, did you know you can sign with your ssh key instead?

gitconfig:

[user]
	name = xxx
	email = xxx
	signingkey = xxxx/.ssh/id_ed25519_sk.pub
[github]
	user = xxx

[commit]
	gpgsign = true

[gpg]
	format = ssh

[CerberusQc]

@apostasie I think I have managed to make the basic squashing happen (feeling super noob here) I will check later for the unit tests to fix the stripping of newline

[apostasie]

@apostasie I think I have managed to make the basic squashing happen (feeling super noob here) I will check later for the unit tests to fix the stripping of newline

Failure is visible on integration tests.

To reproduce locally, make sure your updated nerdctl binary is in your path, then:

go test ./cmd/nerdctl/ -run TestLogsOutStreamsSeparated

(TestLogsOutStreamsSeparated is one of the failing integration test)

[CerberusQc]

@apostasie thank you for guiding me through this, I am really learning today and yes, I prefer SSH signing, I will switch to this in the future.

I have run the whole integration suite in container without any fail, let's see how the pipeline goes

[apostasie]

@apostasie thank you for guiding me through this, I am really learning today and yes, I prefer SSH signing, I will switch to this in the future.

I have run the whole integration suite in container without any fail, let's see how the pipeline goes

That looks promising.
Good stuff here, @CerberusQc.

[CerberusQc]

@apostasie had some trouble making work the unit test and integration test on macOS but, looks good now, I decided to go with the second implementation plus a check on the length of the string, without it the unit tests does not pass for the TestLogsOutStreamsSeparated

[apostasie]

LGTM

Tagging @AkihiroSuda @fahedouch for maintainers' review.

[AkihiroSuda]

Thanks