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.

Sign up for GitHub

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

Jump to bottom

Wait for underlying operation like upload to finish before returning successful message. #30

Closed
Tatsinnit opened this issue Jul 17, 2020 · 2 comments · Fixed by #138
Closed

Wait for underlying operation like upload to finish before returning successful message. #30

Tatsinnit opened this issue Jul 17, 2020 · 2 comments · Fixed by #138
Labels
enhancement 🏎 New feature or request feature-request ⛏☕️

Comments

@Tatsinnit
Copy link
Member

There are many times I have noticed that the aks-periscope command ran successfully but actually the logs get uploaded in decent amount of say 10 to 5 second delay in there storage.

For example: I had more than 3 nodes in my cluster and then once the tool returned successful run status, I went in and checked since my storage explorer was already running in parallel and I had to come back and refresh in few seconds to see all my logs.

the downside of this tool returning success before actually the storage is being uploaded is that it gives false positives and that makes other tool to extend or adopt this tool since the results is not deterministic from the operation of running this command.

💡 Idea: Very likely some re-work is in dire need to fix this internal design level behaviour to reciprocate the difference between successful vs failed status.

Thanks,
@Tatsinnit Tatsinnit mentioned this issue Aug 4, 2020
Merged
@yunjchoi yunjchoi mentioned this issue Sep 29, 2020
Closed
sophsoph321 pushed a commit to sophsoph321/aks-periscope that referenced this issue May 7, 2021
# This is a combination of 127 commits.
2854366 
# This is the 1st commit message:

Revert "introduce arcmode"

This reverts commit 5f4fed4.

# This is the commit message #2:

remove secrets

# This is the commit message Azure#3:

add print statement

# This is the commit message Azure#4:

update print statement

# This is the commit message Azure#5:

committed

# This is the commit message Azure#6:

committed

# This is the commit message Azure#7:

committed

# This is the commit message Azure#8:

committed

# This is the commit message Azure#9:

remove print statements

# This is the commit message Azure#10:

add helm collector

# This is the commit message Azure#11:

change helm command

# This is the commit message Azure#12:

add helm 3 installation

# This is the commit message Azure#13:

add curl command installation

# This is the commit message Azure#14:

change helm command

# This is the commit message Azure#15:

remove helm history

# This is the commit message Azure#16:

debug helm history

# This is the commit message Azure#17:

add repo command

# This is the commit message Azure#18:

change stable repo name

# This is the commit message Azure#19:

add write to file

# This is the commit message Azure#20:

add kured

# This is the commit message Azure#21:

change

# This is the commit message Azure#22:

changes

# This is the commit message Azure#23:

add default namespace

# This is the commit message Azure#24:

change

# This is the commit message Azure#25:

add integration test

# This is the commit message Azure#26:

changes

# This is the commit message Azure#27:

add helm test

# This is the commit message Azure#28:

change print statement to error

# This is the commit message Azure#29:

change

# This is the commit message Azure#30:

more changes

# This is the commit message Azure#31:

add go installation

# This is the commit message Azure#32:

fix unit test

# This is the commit message Azure#33:

iptables to Helm

# This is the commit message Azure#34:

add custom resource collector

# This is the commit message Azure#35:

add new exporter, diagnoser, collector

# This is the commit message Azure#36:

comment unused variables

# This is the commit message Azure#37:

debug exporter

# This is the commit message Azure#38:

filenames

# This is the commit message Azure#39:

test zip function

# This is the commit message Azure#40:

list files

# This is the commit message Azure#41:

fmt to log

# This is the commit message Azure#42:

delete lines

# This is the commit message Azure#43:

changed

# This is the commit message Azure#44:

get current directory

# This is the commit message Azure#45:

remove some print statements

# This is the commit message Azure#46:

test zip

# This is the commit message Azure#47:

changes

# This is the commit message Azure#48:

add windir check

# This is the commit message Azure#49:

minor fix

# This is the commit message Azure#50:

get hostname

# This is the commit message Azure#51:

add expose in dockerfile

# This is the commit message Azure#52:

add exec collector

# This is the commit message Azure#53:

mitigate exit code 126

# This is the commit message Azure#54:

change curl url from example.com to dp endpoint

# This is the commit message Azure#55:

changes

# This is the commit message Azure#56:

uncomment exec

# This is the commit message Azure#57:

add new diagnoser

# This is the commit message Azure#58:

debugging

# This is the commit message Azure#59:

debug

# This is the commit message Azure#60:

debugging

# This is the commit message Azure#61:

remove print statements

# This is the commit message Azure#62:

remove print

# This is the commit message Azure#63:

add back crd print statement

# This is the commit message Azure#64:

change

# This is the commit message Azure#65:

change

# This is the commit message Azure#66:

update dataPoint name

# This is the commit message Azure#67:

modify forloop

# This is the commit message Azure#68:

add filename to datapoint

# This is the commit message Azure#69:

add back log prints

# This is the commit message Azure#70:

test

# This is the commit message Azure#71:

add fields to diagnostic signal

# This is the commit message Azure#72:

add config content to diagnoser

# This is the commit message Azure#73:

change format from yaml to json

# This is the commit message Azure#74:

add parameters for kubeobject config map

# This is the commit message Azure#75:

Revert "introduce arcmode"

This reverts commit 5f4fed4.

# This is the commit message Azure#76:

fix helm collector style

# This is the commit message Azure#77:

revert changes that test arc customizations

# This is the commit message Azure#78:

fix merge conflicts

# This is the commit message Azure#79:

fix merge conflicts

# This is the commit message Azure#80:

Revert "Add v0.3 acr image for Private cluster fix. (Azure#22)"

This reverts commit 49dd302.

# This is the commit message Azure#81:

fix merge conflicts

# This is the commit message Azure#82:

fix merge conflicts

# This is the commit message Azure#83:

add print statement

# This is the commit message Azure#84:

update print statement

# This is the commit message Azure#85:

committed

# This is the commit message Azure#86:

committed

# This is the commit message Azure#87:

committed

# This is the commit message Azure#88:

committed

# This is the commit message Azure#89:

remove print statements

# This is the commit message Azure#90:

fix merge conflicts

# This is the commit message Azure#91:

fix merge conflicts

# This is the commit message Azure#92:

fix merge conflicts

# This is the commit message Azure#93:

add repo command

# This is the commit message Azure#94:

change stable repo name

# This is the commit message Azure#95:

add write to file

# This is the commit message Azure#96:

add kured

# This is the commit message Azure#97:

change

# This is the commit message Azure#98:

changes

# This is the commit message Azure#99:

add default namespace

# This is the commit message Azure#100:

change

# This is the commit message Azure#101:

add integration test

# This is the commit message Azure#102:

changes

# This is the commit message Azure#103:

add helm test

# This is the commit message Azure#104:

change print statement to error

# This is the commit message Azure#105:

change

# This is the commit message Azure#106:

more changes

# This is the commit message Azure#107:

add go installation

# This is the commit message Azure#108:

fix unit test

# This is the commit message Azure#109:

add custom resource collector

# This is the commit message Azure#110:

fix merge conflicts

# This is the commit message Azure#111:

comment unused variables

# This is the commit message Azure#112:

debug exporter

# This is the commit message Azure#113:

filenames

# This is the commit message Azure#114:

test zip function

# This is the commit message Azure#115:

list files

# This is the commit message Azure#116:

fmt to log

# This is the commit message Azure#117:

delete lines

# This is the commit message Azure#118:

changed

# This is the commit message Azure#119:

get current directory

# This is the commit message Azure#120:

remove some print statements

# This is the commit message Azure#121:

test zip

# This is the commit message Azure#122:

changes

# This is the commit message Azure#123:

add windir check

# This is the commit message Azure#124:

minor fix

# This is the commit message Azure#125:

get hostname

# This is the commit message Azure#126:

add expose in dockerfile

# This is the commit message Azure#127:

add exec collector
@Tatsinnit
Copy link
Member Author

Tatsinnit commented Nov 5, 2021 •
edited
Loading

💡 This is debatable reason being, I am just adding thoughts from both sides I guess: (With latest refactors I see this case lesser now)

  • The daemont set operation runs at the background and once the storage is all uploaded the things do appear in storage now this is by design and there is No upfront delay in the apply operation to finish.
  • If we wait then it could be that apply might take some time which is adding delay to the current upfront behaviour.

Just, adding it for more thoughts.

Thanks.

@Tatsinnit Tatsinnit mentioned this issue Dec 8, 2021
Closed
@Tatsinnit Tatsinnit linked a pull request Dec 8, 2021 that will close this issue
Wait for all operations to finish. #137
Closed
@Tatsinnit
Copy link
Member Author

💡 🧙 Some more information to keep this Issue more relevant for right fix to go in, essentially blob.upload is a function which is non-async function but to actually upload the data it does in batches and not all at once. Details here: https://docs.microsoft.com/en-us/azure/storage/blobs/storage-quickstart-blobs-go?tabs=linux#upload-blobs-to-the-container

Read these specific comments from the doc.

// You can use the low-level Upload (PutBlob) API to upload files. Low-level APIs are simple wrappers for the Azure Storage REST APIs.
// Note that Upload can upload up to 256MB data in one shot. Details: https://docs.microsoft.com/rest/api/storageservices/put-blob
// To upload more than 256MB, use StageBlock (PutBlock) and CommitBlockList (PutBlockList) functions.
// Following is commented out intentionally because we will instead use UploadFileToBlockBlob API to upload the blob
// _, err = blobURL.Upload(ctx, file, azblob.BlobHTTPHeaders{ContentType: "text/plain"}, azblob.Metadata{}, azblob.BlobAccessConditions{})
// handleErrors(err)

@Tatsinnit Tatsinnit mentioned this issue Dec 20, 2021
Merged
@Tatsinnit Tatsinnit linked a pull request Dec 20, 2021 that will close this issue
Behaviour fix, Upload API fix. #138
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement 🏎 New feature or request feature-request ⛏☕️
Projects
None yet
Milestone
No milestone
1 participant
@Tatsinnit