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 condor_jobview to show logfile even without indexer. Add api to s… #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mmascher
Copy link

@mmascher mmascher commented Jun 30, 2021

…how logfile from condor id


This change is Reviewable

@@ -253,7 +253,7 @@ def getInfo(self, jobID, given_guid):

# Do directory/file names need to be sanitized?
if given_guid:
cur.execute("SELECT * FROM file_index WHERE GUID='{}'".format(jobID))
cur.execute("SELECT * FROM file_index WHERE GUID LIKE '{}'".format(jobID))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you expect partial GUIDs?
Why otherwise use a LIKE that is generally less performance than a comparison?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, tt could be useful to access logs from condor job id for example. Although now that I implemented the new condor_jobview API this is not needed by CMS anymore.

@@ -24,4 +24,4 @@ def log(error_level, message):
os.makedirs(log_location_dir)
log_location = os.path.join(log_location_dir, datetime.datetime.now().strftime("%Y-%m-%d") + ".txt")
with open(log_location, "a") as log_file:
log_file.write(error_level + " - " + str(int(datetime.datetime.now().timestamp())) + " - " + message + "\n")
log_file.write(error_level + " - " + "{:%Y-%m-%d %H:%M:%S%z}".format(datetime.datetime.now()) + " - " + message + "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

datetime.now is by default naive (no time zone info). To print full iso8601 w/ timezone use:
datetime.datetime.now().astimezone().isoformat() or not to have microseconds datetime.datetime.now().astimezone().replace(microsecond=0).isoformat() and to have a single format string:

log_file.write(f"{error_level} - {datetime.datetime.now().astimezone().replace(microsecond=0).isoformat()} - {message}\n")

def api_condorjob_file(factory, feuser, entry_name, condor_job_id):
# Get configuration

return "/data/glideinmonitor/upload/%s/user_%s/glidein_gfactory_instance/entry_%s/job.%s" % (factory, feuser, entry_name, condor_job_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to use a configuration variable for the root directory. Is this similar to GWMS_Log_Dir except the client subdir?

Comment on lines +4 to +35
function var_win(data, name) {
// Open a variable's contents in a new window
let newWindow = window.open("", name, "width=600, height=800");
newWindow.document.write("<pre>" + data + "</pre>");
newWindow.document.title = name;
}


function var_download(text, filename) {
// Variable downloader
let blob = new Blob([text], {type: "text/plain;charset=utf-8"});
saveAs(blob, filename);
}


function parseTarString(text) {
// Parses a inflated gzip file that's still in a tar? format
// Returns a 2D array containing the name and text of each file
let curr_file = -1;
let content = [];

const lines = text.split('\n');
for (let i = 0; i < lines.length; i++) {
// For each line, check if the line is a TAR file header
if (lines[i].includes(String.fromCharCode(0)) && lines[i].includes("ustar")) {
// Increment the index of the content variable
curr_file += 1;
content[curr_file] = ["", ""];

// Get the file name from the header
let charStart = 0;
while (lines[i].charCodeAt(charStart) === 0) {
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 should be refactored together w/ jobview.js int 3 files to avoid repeating some functions in multiple files.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, should I open a new ticket for this refactoring?

Comment on lines -97 to +117
return api_job_info(job_guid, True)
info = api_job_info(job_guid, True)
return redirect("/job/" + str(json.loads(info)["ID"]), code=303)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you redirect, should there be also a route "/api/job_info/condor_job/..." with a redirect to "/condor_job/...."

Copy link
Author

Choose a reason for hiding this comment

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

This is to have a way to show the job information in a browser given a job_guid (reusing the html page for job). With condor_job there is no such use case IMHO because you only have one way to access the page (the condor job id)

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.

None yet

2 participants